Missions mast #2321

syed-gilani · 2022-03-04T17:35:55Z

PR to fix the following remaining issues for astroquery missions_mast search interface:

Make missions.get_column_list() return an astropy Table.
. Add warning when max number of results are returned (as in mast.catalogues)
Clean up documentation
. Fix capitalization and spelling
. Make sure all links to functions work
. Make quotes around arguments etc consistent with rest of the documentation
. Remove redundant text
. Add more information about when a user should choose this service
. Remove reference to set_service and set_mission functions (they don't exist anymore)
. Make clear what happens when invalid kwargs are passed (that they are just ignored)
Add language in the docstring of services._json_to_table explaining the possible values of col_type and which service(s) they might come from.
Explore the ignore_value handling in services._json_to_table
Consider the way the fall through from using idx to col works in services._json_to_table (line 80 in services.py)
Point user to missions.get_column_list function in missions query doc strings
Make the order of columns in the astropy table returned by the API consistent

pep8speaks · 2022-03-04T17:35:58Z

Hello @syed-gilani! 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 2022-03-15 20:45:01 UTC

bsipocz · 2022-03-07T18:46:53Z

@syed-gilani - It's probably unrelated to the CI status, but I would suggest rebasing this branch now rather than wait with it until all is ready for review in the PR. Rebase is needed as it includes a lot of unrelated commits, many of those are already merged in from the previous PR, or merge commits.

codecov · 2022-03-07T18:52:31Z

Codecov Report

Merging #2321 (222a776) into main (b787ee2) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

❗ Current head 222a776 differs from pull request most recent head 47bd905. Consider uploading reports for the commit 47bd905 to get more accurate results

@@            Coverage Diff             @@
##             main    #2321      +/-   ##
==========================================
- Coverage   62.98%   62.98%   -0.01%     
==========================================
  Files         131      131              
  Lines       17059    17067       +8     
==========================================
+ Hits        10745    10749       +4     
- Misses       6314     6318       +4

Impacted Files	Coverage Δ
astroquery/mast/services.py	`79.56% <ø> (ø)`
astroquery/mast/missions.py	`76.54% <75.00%> (-2.91%)`	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

jaymedina · 2022-03-07T22:16:09Z

astroquery/mast/missions.py

@@ -28,7 +30,7 @@ class MastMissionsClass(MastQueryWithLogin):
    """
    MastMissions search class.

-    Class that allows direct programatic access to the MAST search API for a given mission.
+    Class that allows direct programatic access to retrieve metadata via the MAST search API for a given mission.
    """

    def __init__(self, *, mission='hst', service='search'):


Hi @syed-gilani this PR is looking good, one quick request: can you make the optional arguments keyword arguments only like how you did here?

jaymedina · 2022-03-07T23:06:32Z

astroquery/mast/missions.py

@@ -33,7 +33,7 @@ class MastMissionsClass(MastQueryWithLogin):
    Class that allows direct programatic access to retrieve metadata via the MAST search API for a given mission.
    """

-    def __init__(self, *, mission='hst', service='search'):


No I would have kept that in, that makes the arguments explicit keyword arguments. We've had things break previously when parameters were fed without explicitly stating which kwarg it was

syed-gilani · 2022-03-09T22:07:40Z

@jaymedina This PR is ready to be review. Please review when you have a moment.

jaymedina

Hey @syed-gilani this is looking good! Some minor comments, and I'm going thru the checklist to make sure all the features were added - for anything I didn't check off, could you point me to where that is happening in your code?

astroquery/mast/missions.py

jaymedina · 2022-03-10T18:47:02Z

astroquery/mast/missions.py

-            https://mast.stsci.edu/search/docs/#/Hubble%20Search/post_search_hst_api_v0_1_search_post
+            Any invalid keys are igonerd by the API.
+            All valid key names can be found using `~astroquery.mast.missions.MastMissionsClass.get_column_list`
+            function.
            For example one can specify the output columns(select_cols) or use other filters(conditions)


I'm not sure I understand the wording here, is this saying that there are two key names called select_cols and conditions?

jaymedina · 2022-03-10T19:09:20Z

astroquery/mast/missions.py

        **kwargs
            Mission-specific keyword args.
-            These can be found in the `service documentation <https://mast.stsci.edu/api/v0/_services.html>`__.
-            for specific catalogs. For example one can specify the magtype for an HSC search.
+            Any invalid keys are igonerd by the API.


typo, ignored*

bsipocz · 2022-03-10T19:28:30Z

Please rebase: - there are many (50+?) commits that are unrelated and cover code that has been merged into main already

Please squash - the back and forth of debugging/check on syntax should be squashed out from the history, keep the actual commits to logical chucks rather than to chronological order of trying things.

I think you can address both of these by squashing down to a few commits, or removing all those prior commits and then squashing the rest (both can be done with as an interactive rebase).

syed-gilani · 2022-03-11T14:41:50Z

@bsipocz All the items marked for this PR have been reviewed by @jaymedina. Everything is done. The last unchecked item of inconsistent column ordering in the output astropy table is fixed in our API. Line 54 in astroquery.mast.services.py uses keys in json_obj['info'] to add columns to the astropy table. json_obj['info'] returns inconsistent column order in every API call. We fixed that in our git repo and we will deploy the fix to production early next week. I showed that fix to @jaymedina. You can view the incorrect behavior in our app on production using the following curl command. If make the API call multiple times, you would see a different column ordering in the 'info' key of the json response
curl -X POST -H "Content-Type: application/json" -d '{"target":["M1"],"radius":3,"radius_units":"arcminutes","select_cols":["ang_sep","sci_aper_1234","sci_central_wavelength","sci_data_set_name","sci_dec","sci_actual_duration","sci_spec_1234","sci_hlsp","sci_instrume","sci_pi_last_name","sci_preview_name","sci_pep_id","sci_ra","sci_refnum","sci_release_date","scp_scan_type","sci_start_time","sci_stop_time","sci_targname","sci_status","search_key"],"user_fields":[],"conditions":[{"sci_data_set_name":"JC6801010"},{"sci_obs_type":"all"},{"sci_aec":"S"},{"sci_instrume":"stis,acs,wfc3,cos,fgs,foc,fos,ghrs,hsp,nicmos,wfpc,wfpc2"}],"limit":5000,"offset":0,"sort_by":[],"sort_desc":[],"skip_count":false}' https://mast.stsci.edu/search/hst/api/v0.1/search
We have deployed the fix to dev and you will see the same order of columns every time API call is made
curl -X POST -H "Content-Type: application/json" -d '{"target":["M1"],"radius":3,"radius_units":"arcminutes","select_cols":["ang_sep","sci_aper_1234","sci_central_wavelength","sci_data_set_name","sci_dec","sci_actual_duration","sci_spec_1234","sci_hlsp","sci_instrume","sci_pi_last_name","sci_preview_name","sci_pep_id","sci_ra","sci_refnum","sci_release_date","scp_scan_type","sci_start_time","sci_stop_time","sci_targname","sci_status","search_key"],"user_fields":[],"conditions":[{"sci_data_set_name":"JC6801010"},{"sci_obs_type":"all"},{"sci_aec":"S"},{"sci_instrume":"stis,acs,wfc3,cos,fgs,foc,fos,ghrs,hsp,nicmos,wfpc,wfpc2"}],"limit":5000,"offset":0,"sort_by":[],"sort_desc":[],"skip_count":false}' https://mastdev.stsci.edu/search/hst/api/v0.1/search

bsipocz

I have 2 remaining comment, one about a more transparent handling of kwarg default values, and the other is about the rebase/squash.
And I see that there are still a few unaddressed comments from @jaymedina, please follow-up on those, too.

bsipocz · 2022-03-11T18:56:10Z

astroquery/mast/missions.py


        Returns
        -------
        response : `~astropy.table.Table`
        """

-        return self._service_api_connection._parse_result(response, verbose, data_key='results')
+        results = self._service_api_connection._parse_result(response, verbose, data_key='results')
+        if len(results) >= self.limit:


This is a great approach, and we should in fact do this, or something similar for all other modules :)

bsipocz · 2022-03-11T19:03:44Z

astroquery/mast/missions.py

        coordinates = criteria.pop('coordinates', None)
        objectname = criteria.pop('objectname', None)
        radius = criteria.pop('radius', 0.2*u.deg)
+        self.limit = criteria.pop('limit', 5000)
+        offset = criteria.pop('offset', 0)
+        select_cols = criteria.pop('select_cols', [])


Is there any particular reason that these 6 cannot be pulled out from criteria in the signature? I would think it would be beneficial for the reason to make the defaults visible for the users, and then the 6 lines could be dropped down to 2 in the code itself.

bsipocz · 2022-03-11T20:46:49Z

@syed-gilani - you need to rebase on the main branch of astropy/astroquery rather than the main branch of your fork (thus you ended up with the 700+ commits here).

I'm sure there are people at ST who can help out, but I'm also happy to jump on zoom and do this together step by step, as usually there is just one or two steps that goes wrong and when those are identified the logic of git rebase usually becomes a routine.

syed-gilani · 2022-03-11T22:41:16Z

@bsipocz I think I need help. Let me know if you have a few minutes and we can do it together.

bsipocz · 2022-03-11T22:54:17Z

@syed-gilani - sent you a link in email

syed-gilani · 2022-03-11T23:01:34Z

@bsipocz You forgot to attach the link in the email.

bsipocz · 2022-03-12T00:59:37Z

docs/mast/mast.rst

@@ -1,3 +1,5 @@
+.. doctest-skip-all


I'm afraid you need to revert most of these docs changes, they were mostly came from #1975 that enabled the docs being tested.

This reverts commit e0a6a4d.

syed-gilani · 2022-03-14T15:57:23Z

@bsipocz I reverted that commit and pushed the changes again to the branch. @jaymedina I also made changes based on your feedback.

syed-gilani · 2022-03-15T18:13:32Z

@bsipocz @jaymedina We just deployed the column ordering fix to ops. If you run the code examples from docs, you would get the same order of columns in the generated astropy table

bsipocz · 2022-03-15T18:55:30Z

@jaymedina - I you could do a quick review of this. I think most things got addressed and this is ready to go, pending the one doctest failure I see as the warning should be handled for the test. If that's the only issue remaining I'll fix it prior to merge.

======================================================= FAILURES =======================================================
__________________________________________________ [doctest] mast.rst __________________________________________________
246 
247 for paging through the results, offset and limit can be used to specify the starting record and the number
248 of returned records. the default values for offset and limit is 0 and 5000 respectively.
249 
250 .. doctest-remote-data::
251 
252    >>> from astroquery.mast import MastMissions
253    >>> from astropy.coordinates import SkyCoord
254    >>> missions = MastMissions()
255    >>> results = missions.query_criteria(sci_start_time=">=2021-01-01 00:00:00",
UNEXPECTED EXCEPTION: MaxResultsWarning('Maximum results returned, may not include all sources within radius.')
Traceback (most recent call last):
  File "/Users/bsipocz/.pyenv/versions/3.9.1/lib/python3.9/doctest.py", line 1336, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest mast.rst[38]>", line 1, in <module>
  File "/private/var/folders/dc/hsm7tqpx2d57n7vb3k1l81xw0000gq/T/astroquery-test-ey5y6gcq/lib/python3.9/site-packages/astroquery/utils/class_or_instance.py", line 25, in f
    return self.fn(obj, *args, **kwds)
  File "/private/var/folders/dc/hsm7tqpx2d57n7vb3k1l81xw0000gq/T/astroquery-test-ey5y6gcq/lib/python3.9/site-packages/astroquery/utils/process_asyncs.py", line 29, in newmethod
    result = self._parse_result(response, verbose=verbose)
  File "/private/var/folders/dc/hsm7tqpx2d57n7vb3k1l81xw0000gq/T/astroquery-test-ey5y6gcq/lib/python3.9/site-packages/astroquery/mast/missions.py", line 67, in _parse_result
    warnings.warn("Maximum results returned, may not include all sources within radius.",
astroquery.exceptions.MaxResultsWarning: Maximum results returned, may not include all sources within radius.
/private/var/folders/dc/hsm7tqpx2d57n7vb3k1l81xw0000gq/T/astroquery-test-ey5y6gcq/lib/python3.9/site-packages/docs/mast/mast.rst:255: UnexpectedException
=============================================== short test summary info ================================================
FAILED docs/mast/mast.rst::mast.rst
================================= 1 failed, 85 passed, 1 skipped in 572.19s (0:09:32) ==================================

jaymedina

Just had one minor comment left over but not a big deal so I will mark this as approved. PR looks great, thanks for your work

jaymedina · 2022-03-15T19:04:50Z

astroquery/mast/services.py

@@ -50,6 +50,7 @@ def _json_to_table(json_obj, data_key='data'):

    # for each item in info, store the type and column name
    # for each item in info, type has to be converted from DB data types (SQL server in most cases)
+    # from missions_mast search service such as varchar, integer, float, boolean etc


could you be explicit here and mention that these keywords you are searching with are the cols?

…Fix once issue 2323 is addressed

bsipocz · 2022-03-15T20:50:12Z

OK, so my commit is a very ugly hack to work around #2323, yet keep some ways to show the warning in the example. Once we fix the resource warning, this last commit should be changed to # doctest: +SHOW_WARNINGS and then have the warning as an output line rather than a commented out command line.

Otherwise, it all passes remote tests, so I go ahead with the merge now. Thank you @syed-gilani and @jaymedina for other review.

bsipocz · 2022-03-15T20:52:25Z

@jaymedina - someone else asked for a release recently, so now I wonder whether you think all the mast related follow-ups are done? It's OK if there are still release blocking things, we can milestone them so I won't forgot about them, I just can't recall anything else than these mission follow-ups.

jaymedina · 2022-03-15T21:02:08Z

@jaymedina - someone else asked for a release recently, so now I wonder whether you think all the mast related follow-ups are done? It's OK if there are still release blocking things, we can milestone them so I won't forgot about them, I just can't recall anything else than these mission follow-ups.

There are no mast related standing issues left. We're at the end of our quarter (1.5 weeks left) and all major changes have been wrapped up and merged. I have some open tickets under the Issues tab, but those don't have a milestone or a deadline attached to them, so I will work on them for the following release.

bsipocz · 2022-03-15T21:07:19Z

Sounds great. In that case, I'll try to cut a release next week, we have plenty of new materials in the milestone already.

bsipocz added the mast label Mar 4, 2022

bsipocz requested a review from jaymedina March 4, 2022 18:16

bsipocz added this to the v0.4.6 milestone Mar 4, 2022

jaymedina reviewed Mar 7, 2022

View reviewed changes

syed-gilani marked this pull request as ready for review March 8, 2022 21:16

jaymedina suggested changes Mar 10, 2022

View reviewed changes

jaymedina reviewed Mar 10, 2022

View reviewed changes

bsipocz reviewed Mar 11, 2022

View reviewed changes

updates to missions_mast search API astroquery interface

e0a6a4d

syed-gilani force-pushed the missions_mast branch from 8199cee to e0a6a4d Compare March 12, 2022 00:47

bsipocz reviewed Mar 12, 2022

View reviewed changes

syed-gilani added 2 commits March 14, 2022 11:02

Revert "updates to missions_mast search API astroquery interface"

391ecaa

This reverts commit e0a6a4d.

updates to missions_mast astroquery interface

6bc272d

made radius keyword argument in docs

222a776

bsipocz added the Affects-dev label Mar 15, 2022

bsipocz mentioned this pull request Mar 15, 2022

Additional MAST docs test update #2315

Merged

jaymedina approved these changes Mar 15, 2022

View reviewed changes

Super ugly hack to get the warning shown but still pass the doctest. …

47bd905

…Fix once issue 2323 is addressed

bsipocz force-pushed the missions_mast branch from 947ab37 to 47bd905 Compare March 15, 2022 20:44

bsipocz approved these changes Mar 15, 2022

View reviewed changes

bsipocz merged commit af6b41b into astropy:main Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missions mast #2321

Missions mast #2321

syed-gilani commented Mar 4, 2022

pep8speaks commented Mar 4, 2022 •

edited

bsipocz commented Mar 7, 2022

codecov bot commented Mar 7, 2022 •

edited

jaymedina Mar 7, 2022

jaymedina Mar 7, 2022

syed-gilani commented Mar 9, 2022

jaymedina left a comment •

edited

jaymedina Mar 10, 2022

jaymedina Mar 10, 2022

bsipocz commented Mar 10, 2022

syed-gilani commented Mar 11, 2022

bsipocz left a comment

bsipocz Mar 11, 2022

bsipocz Mar 11, 2022

bsipocz commented Mar 11, 2022

syed-gilani commented Mar 11, 2022

bsipocz commented Mar 11, 2022

syed-gilani commented Mar 11, 2022

bsipocz Mar 12, 2022

syed-gilani commented Mar 14, 2022

syed-gilani commented Mar 15, 2022

bsipocz commented Mar 15, 2022

jaymedina left a comment

jaymedina Mar 15, 2022

bsipocz commented Mar 15, 2022

bsipocz commented Mar 15, 2022

jaymedina commented Mar 15, 2022

bsipocz commented Mar 15, 2022

Missions mast #2321

Missions mast #2321

Conversation

syed-gilani commented Mar 4, 2022

pep8speaks commented Mar 4, 2022 • edited

Comment last updated at 2022-03-15 20:45:01 UTC

bsipocz commented Mar 7, 2022

codecov bot commented Mar 7, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syed-gilani commented Mar 9, 2022

jaymedina left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Mar 10, 2022

syed-gilani commented Mar 11, 2022

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Mar 11, 2022

syed-gilani commented Mar 11, 2022

bsipocz commented Mar 11, 2022

syed-gilani commented Mar 11, 2022

Choose a reason for hiding this comment

syed-gilani commented Mar 14, 2022

syed-gilani commented Mar 15, 2022

bsipocz commented Mar 15, 2022

jaymedina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Mar 15, 2022

bsipocz commented Mar 15, 2022

jaymedina commented Mar 15, 2022

bsipocz commented Mar 15, 2022

pep8speaks commented Mar 4, 2022 •

edited

codecov bot commented Mar 7, 2022 •

edited

jaymedina left a comment •

edited