Parameter validation on MastMissions queries #3126

snbianco · 2024-11-05T18:56:17Z

Checks the validity of criteria keyword arguments on MastMissions queries. Raises an InvalidQueryError if an invalid parameter is passed rather then sending them to the API to be ignored. If the invalid parameter is close to a valid keyword, the user is prompted with the closest match.

I also altered the MastMissions.get_column_list method to save the column list for each mission in a dictionary class attribute called columns. This way, we do not make unnecessary API calls.

I added a test to test_mast_remote.py and modified some in test_mast.py that were not passing in arguments correctly.

@dr-rodriguez @astrojimig

snbianco · 2024-11-05T19:04:34Z

There are a few tests that might fail due to an unrelated issue that we've been having with PS1 queries. If they happen, the failures would be for the Catalogs.query_criteria function where catalog='panstarrs'. I'm working on getting this sorted out.

codecov · 2024-11-05T19:38:16Z

Codecov Report

Attention: Patch coverage is 71.28713% with 29 lines in your changes missing coverage. Please review.

Project coverage is 67.53%. Comparing base (701a4e3) to head (ed3bdff).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/mast/collections.py	73.77%	16 Missing ⚠️
astroquery/mast/missions.py	69.44%	11 Missing ⚠️
astroquery/mast/utils.py	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3126      +/-   ##
==========================================
+ Coverage   67.49%   67.53%   +0.04%     
==========================================
  Files         233      233              
  Lines       18420    18487      +67     
==========================================
+ Hits        12433    12486      +53     
- Misses       5987     6001      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bsipocz

Some minor comments in line and bigger picture ones:

I think it may be useful for the users to have a method that actually lists all the possibly kwargs?
I'm sure there are a couple of related open issues for this as weeding out the everything swallowing **kwargs is a thing I would have liked to see for a while. Could you please cross link those, so we can close a couple of things? So, overall thank you.

bsipocz · 2024-11-05T20:18:15Z

astroquery/mast/missions.py

+
+        Parameters
+        ----------
+        **kwargs


Suggested change

**kwargs

**criteria

bsipocz · 2024-11-05T20:20:11Z

astroquery/mast/missions.py

+                # Create Table with parsed data
+                col_table = Table(rows=rows, names=('name', 'data_type', 'description'))
+                self.columns[self.mission] = col_table
+            except Exception:


I see that this is inherited code, but we should be more specific and only except specific exceptions rather than staying this super general. And if we need to stay this general, capture the exception and raise the same more specific type of it rather than reraise the generic Exception

Modified to catch more specific exceptions

bsipocz · 2024-11-05T20:21:14Z

astroquery/mast/tests/data/mission_columns.json

Is there a way to programatically generate this file? If yes, does it take long?
(I'm asking as files like this tend to diverge from actual server side behaviour super quickly)

This file is only being used for the non-remote test suite, and programmatically generating it would require a connection. Because it's describing a table schema that already exists (and the query results are being mocked as well), I don't think we should have any issues with server-side changes.

OK. Then just maybe add a readme file in the data directory that describes how to generate this file, if needed, in the future.

snbianco · 2024-11-05T21:43:40Z

Some minor comments in line and bigger picture ones:

I think it may be useful for the users to have a method that actually lists all the possibly kwargs?

I'm sure there are a couple of related open issues for this as weeding out the everything swallowing **kwargs is a thing I would have liked to see for a while. Could you please cross link those, so we can close a couple of things? So, overall thank you.

For your first point, there is the MastMissions.get_column_list method that returns a table of valid columns for the relevant mission. For the second, I've linked some related issues:

DOC: make signatures more explicit in MAST docstrings #2665 - This sounds like it needs to be addressed for the entire module. Should I do that in this PR or just stick to the MastMissions class and address the rest later?
MAST: Missions MAST Followup Issues #2298 - Some of the checklist items have been addressed here, but not all.
Limit usage of **kwargs to raise error for non functional kwargs of the common API #2109 - Relevant, but this is still an issue for the Catalogs.query_object function

bsipocz · 2024-11-05T21:52:13Z

Should I do that in this PR or just stick to the MastMissions class and address the rest later?

It's OK to do that in a follow up. Also, given that there are 50+ options, I think it's a good middle ground to explicitly refer to the method that lists the possible options rather than actually adding them to the docstring.

Relevant, but this is still an issue for the Catalogs.query_object function

OK, so if something similar could be done for catalogs, too then we can at least cross that topic off for mast. I'll still need to address it for any of the other modules that do it.

snbianco · 2024-11-06T15:06:16Z

I made some changes in Catalogs.query_region to do parameter checking, so the issue in the description of #2109 should be fixed.

bsipocz

Thank you! One remaining comment about validating params for query_region, too, otherwise it all looks good.

bsipocz · 2024-11-06T16:58:53Z

astroquery/mast/collections.py

@@ -109,10 +110,18 @@ def query_region_async(self, coordinates, *, radius=0.2*u.deg, catalog="Hsc",
                  'dec': coordinates.dec.deg,
                  'radius': radius.deg}

+        # valid criteria keywords
+        valid_criteria = []


Nitpick, but I would move this into the conditional as you only use it in the else part

bsipocz · 2024-11-06T17:02:28Z

astroquery/mast/collections.py

        # Determine API connection and service name
        if catalog.lower() in self._service_api_connection.SERVICES:
            self._current_connection = self._service_api_connection
            service = catalog
+
+            # adding additional user specified parameters
+            for prop, value in criteria.items():


Any chance to validate these, or there is way to large a scatter in what's allowed for each of these catalogs?

So, the only service that runs through Catalogs.MAST (currently), and thus the service API connection, is PanSTARRS. I added Catalogs._get_service_col_config and Catalogs._validate_service_criteria to fetch the column metadata for PanSTARRS and check parameters.

snbianco · 2024-11-06T22:16:11Z

The last commit is a super quick add for getting cloud URIs of HLSPs!

Mock MastMissions.get_column_list Increase code coverage

…n collections

fix JSONDecodeError import

test case, changelog

bsipocz

Thanks, this looks good now!

snbianco added the mast label Nov 5, 2024

snbianco marked this pull request as ready for review November 5, 2024 20:15

snbianco requested a review from bsipocz November 5, 2024 20:16

bsipocz reviewed Nov 5, 2024

View reviewed changes

bsipocz reviewed Nov 6, 2024

View reviewed changes

bsipocz added this to the v0.4.8 milestone Nov 6, 2024

snbianco added 5 commits November 6, 2024 17:31

Parameter checking on Missions queries

d3ecf0d

Mock MastMissions.get_column_list Increase code coverage

Rename **kwargs, info to regenerate column file, parameter checking o…

9d659e7

…n collections

Better exception handling, fix README code

18772c5

fix JSONDecodeError import

Validate panstarrs criteria

30c69ee

Get cloud URIs for HLSPs

ed3bdff

test case, changelog

snbianco force-pushed the ASB-29312-missions-params branch from 6a1dbe1 to ed3bdff Compare November 6, 2024 22:31

bsipocz approved these changes Nov 6, 2024

View reviewed changes

bsipocz merged commit e150df1 into astropy:main Nov 6, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter validation on MastMissions queries #3126

Parameter validation on MastMissions queries #3126

snbianco commented Nov 5, 2024 •

edited

Loading

snbianco commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

bsipocz left a comment

bsipocz Nov 5, 2024

bsipocz Nov 5, 2024

snbianco Nov 6, 2024

bsipocz Nov 5, 2024

snbianco Nov 5, 2024 •

edited

Loading

bsipocz Nov 5, 2024

snbianco commented Nov 5, 2024

bsipocz commented Nov 5, 2024

snbianco commented Nov 6, 2024

bsipocz left a comment

bsipocz Nov 6, 2024

bsipocz Nov 6, 2024

snbianco Nov 6, 2024

snbianco commented Nov 6, 2024

bsipocz left a comment

Parameter validation on MastMissions queries #3126

Parameter validation on MastMissions queries #3126

Conversation

snbianco commented Nov 5, 2024 • edited Loading

snbianco commented Nov 5, 2024 • edited Loading

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snbianco Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snbianco commented Nov 5, 2024

bsipocz commented Nov 5, 2024

snbianco commented Nov 6, 2024

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

Choose a reason for hiding this comment

snbianco commented Nov 6, 2024

bsipocz left a comment

Choose a reason for hiding this comment

snbianco commented Nov 5, 2024 •

edited

Loading

snbianco commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

snbianco Nov 5, 2024 •

edited

Loading