get_obs_products method supports product_type parameter as string or list #2995

davidglt · 2024-04-23T11:58:45Z

The get_obs_products method now checks the input argument product_type with the method _check_list_strings and allows using strings or lists

pep8speaks · 2024-04-23T11:58:52Z

Hello @davidglt! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-07-16 17:46:57 UTC

jespinosaar · 2024-04-23T12:51:53Z

cc @esdc-esac-esa-int

davidglt · 2024-04-25T09:58:55Z

Hi there!
The last commit 61b8b14 only modifies from the file in the main branch the lines to include this PR information, but maybe I have something more wrong :)
Cheers,
David

jespinosaar · 2024-04-25T15:46:25Z

Hi @davidglt , please rebase your branch. It seems there is a conflict in CHANGES.rst file.

davidglt · 2024-04-26T09:55:25Z

Hi there!
After rebase, we have the same problem with the CHANGES.rst file

Cheers,
David

bsipocz · 2024-05-03T02:06:02Z

I'm not exactly sure what went wrong with the rebase here, I suspect it might not been rebased on the correct branch. Given that there are new conflicts I do a rebase again and will report back the command history I will have used.

bsipocz · 2024-05-03T02:29:55Z

Given that there were a lot of incorrect edits for the changelog, I removed those commits during the rebase and instead added a single clean on once the rest has been rebased. Here are the commands:

Make sure you have the astropy/astroquery repo added as a git remote (I call this astropy in my setup, but it maybe upstream in yours):

$ ~/munka/devel/astroquery [ESA_jwst-extend_get_obs_products|✚ 1…1⚑ 14] $ git remote -v
astropy	[email protected]:astropy/astroquery.git (fetch)
astropy	[email protected]:astropy/astroquery.git (push)
bsipocz	[email protected]:bsipocz/astroquery.git (fetch)
bsipocz	[email protected]:bsipocz/astroquery.git (push)
esdc-esac-esa-int	[email protected]:esdc-esac-esa-int/astroquery (fetch)
esdc-esac-esa-int	[email protected]:esdc-esac-esa-int/astroquery (push)

Have a fresh version of the main repo:

git fetch --all --prune

Interactive rebase on astropy/main. This will automatically get rid of the ~80 commits that you pulled in during the previous rebase. Also, in the interactive mode an editor will pop up. I removed all commits related to the changelog. The rebase was clean, I didn't need to resolve any conflicts, but if there are any, you read the instructions and do --continue until all commits are successfully rebased.

git rebase -i astropy/main

Once the rebase is done, I edited the changelog file and added that commit.

Now, the important part (which might have been the step that didn't worked previously) is to force push the branch back to github. I call the ESAC fork push esdc-esac-esa-int, you may have it as origin.

git push esdc-esac-esa-int HEAD:ESA_jwst-extend_get_obs_products -f

bsipocz

Please add a test that covers the list case for product_type input. Also see the inline comments as I suspect the a bug, but also suggest a shorter solution for the enhancement.

bsipocz · 2024-05-03T02:34:51Z

astroquery/esa/jwst/core.py

@@ -83,6 +83,15 @@ def __init__(self, *, tap_plus_handler=None, data_handler=None, show_messages=Tr
        if show_messages:
            self.get_status_messages()

+    def __check_list_strings(self, list):


I would strongly advise calling the parameter something else than list to avoid overriding the list()

bsipocz · 2024-05-03T02:36:20Z

astroquery/esa/jwst/core.py

+    def __check_list_strings(self, list):
+        if list is None:
+            return False
+        if list and all(isinstance(elem, str) for elem in list):


a single string is iterable and this will return True, is that the logic you want to do here?

bsipocz · 2024-05-03T02:40:50Z

astroquery/esa/jwst/core.py

+
+        if self.__check_list_strings(product_type):
+            if type(product_type) is list:
+                tap_product_type = ",".join(str(elem) for elem in product_type)
+            else:
+                tap_product_type = product_type
+        else:
+            tap_product_type = None


this function is only used in this one place, so I would instead suggest something in-place here along the line of the following few line. Arguably this doesn't gracefully handle the case when the input list is not full of strings, but the docs calls for strings, so duct-typeing should be sufficient. And with this the extra private function is not needed either.

Suggested change

if self.__check_list_strings(product_type):

if type(product_type) is list:

tap_product_type = ",".join(str(elem) for elem in product_type)

else:

tap_product_type = product_type

else:

tap_product_type = None

if isinstance(product_type, list):

tap_product_type = ",".join(str(elem) for elem in product_type)

elif isinstance(product_type, str):

tap_product_type = product_type

else:

tap_product_type = None

Brigitta, thank you very much for your comments and patience!
I was not using the --force in the push, my fault :)
About this method "check_list_strings" is used as in other submodules (ex. hubble), but you’re right, in jwst no, I can delete it and check the entries directly in "get_obs_products".
Cheers,
David

bsipocz · 2024-05-03T03:00:57Z

Also, @davidglt - if you add the email address you made the commits with to your github profile, these commits will properly show up as your contributions.

davidglt · 2024-05-06T12:23:45Z

Hi Brigitta, I am working to implement a new test for the product_type parameter as string or list

davidglt · 2024-05-20T09:28:23Z

Hi Brigitta,
I've included a remote-test in docs/esa/jwst/jwst.rst with product_type as a list.
Do you need anything else?

Thanks!
David

jespinosaar · 2024-05-20T17:02:41Z

Thanks @davidglt! we still need to include the entry in the changelog.

bsipocz · 2024-05-20T23:14:10Z

Do you need anything else?

I'm travelling in the coming week so PR reviews and merges may be delayed. Sorry about that.
With a quick look at the changes, I see that a test is still missing (# doctest: +SKIP skips running the code example). And the changelog changes will need to be squashed, but I'm more than happy to do that once I'm back in Seattle.

davidglt · 2024-05-21T09:20:11Z

Hi Brigitta,
Now we have a green light! maybe the interpreter was confused :)
I learned a lot from your suggestions!
Cheers,
David

davidglt · 2024-05-31T09:28:29Z

Hi Brigitta,
Do you need anything more?
Cheers,
David

bsipocz

Please add a test for product_type being a list.

astroquery/esa/jwst/core.py

jespinosaar · 2024-06-18T15:52:06Z

Please add a test for product_type being a list.

Hi @davidglt , I will help you with the test.

davidglt · 2024-07-02T12:01:44Z

Hello!
We have just implemented your two recommendations:

Replace "type(product_type is list" with isinstance(product_type, list)
Improve comments on the get_obs_products method for the output parameter

Cheers,
David

davidglt · 2024-07-02T12:13:57Z

Please add a test for product_type being a list.

We have done an online test in docs/esa/jwst/jwst.rst, do you want an offline test too or is that enough?
Cheers,
David

davidglt · 2024-07-08T09:41:06Z

Please add a test for product_type being a list.

We have done an online test in docs/esa/jwst/jwst.rst, do you want an offline test too or is that enough? Cheers, David

Hi Brigitta,
test offline for product_type as a list in test_get_obs_products included.
Cheers,
David

jespinosaar · 2024-07-08T12:36:33Z

Hi @bsipocz , I have seen the test is now included. I have checked it and seems to be working. The issue with the CI seems to be unrelated. Thanks @davidglt

bsipocz

Some minor comments about the tests, happy to merge as is and leave that for a future cleanup, or wait here.

bsipocz · 2024-07-11T19:26:57Z

astroquery/esa/jwst/core.py

@@ -997,10 +1000,16 @@ def get_obs_products(self, *, observation_id=None, cal_level="ALL",
                                                max_cal_level=max_cal_level,
                                                is_url=True)
        params_dict['planeid'] = plane_ids
+
+        if type(product_type) is list:


Suggested change

if type(product_type) is list:

if isinstance(product_type, list):

bsipocz · 2024-07-11T19:29:19Z

astroquery/esa/jwst/tests/test_jwsttap.py

@@ -733,6 +733,37 @@ def test_get_obs_products(self):
        finally:
            shutil.rmtree(output_file_full_path_dir)

+        # Test product_type paramater with a list
+        output_file_full_path_dir = os.getcwd() + os.sep + "temp_test_jwsttap_get_obs_products_1"


I see that all these are inherited from above, so these are generic cleanup comments, not necessary to address them for merging this particular PR:

os.path.join does this much nicer than creating the output string like this.

the try except below is not really necessary for the tests. If the test fail with OSError, then I presume the developer/maintainer knows how to read the traceback.

It may be worth to do parametrization of the product_type instead of copying practically the same code (e.g. look for the usage of pytest.mark.parametrize in e.g the esasky tests)

overall, it is better if tests are made smaller, so e.g. here if there is copied code, it may instead land in a separate test as opposed to be added after an already existing one.

jespinosaar · 2024-07-16T10:43:36Z

Hi @bsipocz ,
Many thanks for your feedback! We will be cleaning the code for all ESA modules in a different pull request, so maybe we can merge this PR and then create another one with changes for all of them. What do you think?

bsipocz · 2024-07-16T17:42:02Z

@jespinosaar - Yes, that sounds good to me. I'll rebase to fix the conflict, and then merge this one as is.

…list

…he product_type attribute

…) in get_obs_products method. Improve the product_type comments in get_obs_products method.

bsipocz · 2024-07-16T18:12:59Z

Thank you!

jespinosaar added the esa.jwst label Apr 23, 2024

jespinosaar added this to the v0.4.8 milestone Apr 23, 2024

davidglt pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2024

Modify CHANGES.rst with astropy#2995 PR

0309fc3

davidglt pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2024

Modify CHANGES.rst with astropy#2995 PR

e97bfa2

davidglt pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2024

Modify CHANGES.rst with astropy#2995 PR

03093c6

davidglt pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2024

Modify CHANGES.rst with astropy#2995 PR

e4cf7eb

davidglt pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request Apr 25, 2024

Modify CHANGES.rst with astropy#2995 PR

61b8b14

bsipocz force-pushed the ESA_jwst-extend_get_obs_products branch from 2d0bebd to 7b88527 Compare May 3, 2024 02:21

bsipocz requested changes May 3, 2024

View reviewed changes

davidglt force-pushed the ESA_jwst-extend_get_obs_products branch from 7b88527 to 6e9dfbd Compare May 6, 2024 07:57

davidglt force-pushed the ESA_jwst-extend_get_obs_products branch from 78d03ec to d7bcbbc Compare May 21, 2024 08:32

bsipocz requested changes Jun 11, 2024

View reviewed changes

astroquery/esa/jwst/core.py Outdated Show resolved Hide resolved

astroquery/esa/jwst/core.py Outdated Show resolved Hide resolved

bsipocz approved these changes Jul 12, 2024

View reviewed changes

David Gonzalez and others added 11 commits July 16, 2024 10:42

get_obs_products method supports product_type parameter as string or …

95bc9fe

…list

align to PEP8

e0b6649

Modify CHANGES.rst file with get_obs_products method changes

4d2a204

Delete check_list_strings method

b9f52d1

cleaner get_obs_products method

21d6c21

clean whitespaces

a10a87b

remote test for product_type parameter as list

89c7d3b

delete "# doctest: +SKIP" from get_obs_products tests when they use t…

667d254

…he product_type attribute

Replace "type(product_type) is list" by isinstance(product_type, list…

ec3f48b

…) in get_obs_products method. Improve the product_type comments in get_obs_products method.

Include offline test for product_type as a list

c1f0855

Style fixes

bb734e0

bsipocz force-pushed the ESA_jwst-extend_get_obs_products branch from d0f7e1a to bb734e0 Compare July 16, 2024 17:46

bsipocz merged commit 08ff1b8 into astropy:main Jul 16, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get_obs_products method supports product_type parameter as string or list #2995

get_obs_products method supports product_type parameter as string or list #2995

davidglt commented Apr 23, 2024

pep8speaks commented Apr 23, 2024 •

edited

Loading

jespinosaar commented Apr 23, 2024

davidglt commented Apr 25, 2024

jespinosaar commented Apr 25, 2024

davidglt commented Apr 26, 2024

bsipocz commented May 3, 2024

bsipocz commented May 3, 2024

bsipocz left a comment

bsipocz May 3, 2024

bsipocz May 3, 2024

bsipocz May 3, 2024

davidglt May 6, 2024

bsipocz commented May 3, 2024

davidglt commented May 6, 2024

davidglt commented May 20, 2024

jespinosaar commented May 20, 2024

bsipocz commented May 20, 2024

davidglt commented May 21, 2024

davidglt commented May 31, 2024

bsipocz left a comment

jespinosaar commented Jun 18, 2024 •

edited

Loading

davidglt commented Jul 2, 2024

davidglt commented Jul 2, 2024

davidglt commented Jul 8, 2024

jespinosaar commented Jul 8, 2024 •

edited

Loading

bsipocz left a comment

bsipocz Jul 11, 2024

bsipocz Jul 11, 2024

jespinosaar commented Jul 16, 2024

bsipocz commented Jul 16, 2024 •

edited

Loading

bsipocz commented Jul 16, 2024

	if type(product_type) is list:
	if isinstance(product_type, list):

get_obs_products method supports product_type parameter as string or list #2995

get_obs_products method supports product_type parameter as string or list #2995

Conversation

davidglt commented Apr 23, 2024

pep8speaks commented Apr 23, 2024 • edited Loading

Comment last updated at 2024-07-16 17:46:57 UTC

jespinosaar commented Apr 23, 2024

davidglt commented Apr 25, 2024

jespinosaar commented Apr 25, 2024

davidglt commented Apr 26, 2024

bsipocz commented May 3, 2024

bsipocz commented May 3, 2024

bsipocz left a comment

Choose a reason for hiding this comment

bsipocz May 3, 2024

Choose a reason for hiding this comment

bsipocz May 3, 2024

Choose a reason for hiding this comment

bsipocz May 3, 2024

Choose a reason for hiding this comment

davidglt May 6, 2024

Choose a reason for hiding this comment

bsipocz commented May 3, 2024

davidglt commented May 6, 2024

davidglt commented May 20, 2024

jespinosaar commented May 20, 2024

bsipocz commented May 20, 2024

davidglt commented May 21, 2024

davidglt commented May 31, 2024

bsipocz left a comment

Choose a reason for hiding this comment

jespinosaar commented Jun 18, 2024 • edited Loading

davidglt commented Jul 2, 2024

davidglt commented Jul 2, 2024

davidglt commented Jul 8, 2024

jespinosaar commented Jul 8, 2024 • edited Loading

bsipocz left a comment

Choose a reason for hiding this comment

bsipocz Jul 11, 2024

Choose a reason for hiding this comment

bsipocz Jul 11, 2024

Choose a reason for hiding this comment

jespinosaar commented Jul 16, 2024

bsipocz commented Jul 16, 2024 • edited Loading

bsipocz commented Jul 16, 2024

pep8speaks commented Apr 23, 2024 •

edited

Loading

jespinosaar commented Jun 18, 2024 •

edited

Loading

jespinosaar commented Jul 8, 2024 •

edited

Loading

bsipocz commented Jul 16, 2024 •

edited

Loading