Support expressions when filtering products by numeric columns #3365
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3365 +/- ##
==========================================
+ Coverage 70.07% 70.16% +0.08%
==========================================
Files 232 232
Lines 19893 19918 +25
==========================================
+ Hits 13940 13975 +35
+ Misses 5953 5943 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
astroquery/mast/missions.py
Outdated
| col_mask = np.isin(products[colname], vals) | ||
| col_data = products[colname] | ||
| # If the column is an integer or float, accept numeric filters | ||
| if col_data.dtype.kind in 'if': |
There was a problem hiding this comment.
in 'if'? I'm not sure I get that
There was a problem hiding this comment.
It's checking whether the kind code of the column is i (integer) or f (float).
There was a problem hiding this comment.
Can you add that as a comment? Or even better, rephrase the line as
| if col_data.dtype.kind in 'if': | |
| if col_data.dtype.kind in ["i", "f"]: |
astroquery/mast/missions.py
Outdated
| except ValueError: | ||
| warnings.warn(f"Could not parse numeric filter '{vals}' for column '{colname}'.", InputWarning) | ||
| continue |
There was a problem hiding this comment.
why not allow the exception to be raised here?
There was a problem hiding this comment.
Agreed that this should just raise an exception.
astroquery/mast/observations.py
Outdated
| if colname not in products.colnames: | ||
| warnings.warn(f"Column '{colname}' not found in product table.", InputWarning) | ||
| continue |
There was a problem hiding this comment.
should this be an exception instead?
There was a problem hiding this comment.
My reasoning here was that the schema of the product table returned by our API may change in the future. Only issuing a warning would prevent a user's code from breaking unexpectedly, but I suppose the output of the function would not be what the user expects either. This is also the precedent set by the MastMissions class.
There was a problem hiding this comment.
Yeap, but if the API is changing then their code will have to change anyway, or the astroquery module should change and thus they have to update the astroquery version to keep the same user code.
There was a problem hiding this comment.
I see where you're coming from! It probably would be better to fully alert the user if the column they have been filtering on no longer exists. The latest commit raises an error in both Observations and MastMissions if a nonexistent column filter is passed.
astroquery/mast/tests/test_mast.py
Outdated
| # Filter by extension | ||
| filtered = mast.MastMissions.filter_products(products, | ||
| extension='fits') |
There was a problem hiding this comment.
nitpick, but we allow linelength to run up to 120, there is really no need for the break here, and below
Enhanced
filter_productsmethods inMastMissionsClassandObservationsClassto support advanced filtering expressions for numeric columns. Users can now filter using single values, value ranges (e.g.,"100..1000"), comparison operators (e.g.,">=500"), or lists combining multiple expressions (e.g.,[100, "500..1000", ">=1500"]). This provides greater flexibility when filtering mission data products.Also includes tests and documentation updates. Fixed some unrelated documentation tests that were out-of-date.