Handle non-ICRS coordinates in query functions #3164
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3164 +/- ##
=======================================
Coverage 70.07% 70.07%
=======================================
Files 232 232
Lines 19893 19896 +3
=======================================
+ Hits 13940 13943 +3
Misses 5953 5953 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
snbianco
force-pushed
the
ASB-29990-coord-frame
branch
from
4dc257f to
f2bd777
Compare
snbianco
force-pushed
the
ASB-29990-coord-frame
branch
from
4baa044 to
50648fd
Compare
There was a problem hiding this comment.
I wonder if this should live in the commons function, or you could use what we already have for simbad (_parse_coordinate_and_convert_to_icrs)?
(or also move the simbad one to commons? cc @ManonMarchand )
_parse_coordinate_and_convert_to_icrs
astroquery/mast/utils.py
Outdated
| @@ -121,6 +121,32 @@ def resolve_object(objectname): | |||
| return coordinates | |||
|
|
|||
|
|
|||
| def parse_coordinates(coordinates): | |||
There was a problem hiding this comment.
I wonder if this should be a kwarg argument for commons.parse_coordinates instead? e.g. to convert right at the end before returning the current output
There was a problem hiding this comment.
Do you mean a parameter like return_frame or something to that effect that you could pass in a value like 'ICRS' and get the coordinate back in whatever frame was given?
There was a problem hiding this comment.
Just made this change! I added a return_frame parameter to utils.commons.parse_coordinates that defaults to None. If it is provided and the coordinates are not already in that frame, it will attempt to transform them. If the transformation fails, a warning is issued and the coordinates are returned as is, without the transformation.
|
Oh that methods only parses coordinates if I detect that they are written as a sim-script string. This is an old way of writing coordinates that only existed in the former Simbad interface (hence this is in Simbad's utils and not in commons). |
snbianco
force-pushed
the
ASB-29990-coord-frame
branch
from
f83c591 to
0746ddf
Compare
snbianco
force-pushed
the
ASB-29990-coord-frame
branch
2 times, most recently
from
69ca4b1 to
42597be
Compare
|
@bsipocz Pinging this PR again in case it was lost in the shuffle somewhere! I'll be offline from 3/26 - 4/3, but this isn't high priority, so no worries if you don't get around to it before then. |
astroquery/utils/commons.py
Outdated
| @@ -40,7 +40,7 @@ | |||
| ASTROPY_LT_7_1_1 = not minversion('astropy', '7.1.1') | |||
|
|
|||
|
|
|||
| def parse_coordinates(coordinates): | |||
| def parse_coordinates(coordinates, return_frame=None): | |||
There was a problem hiding this comment.
make this mandatory kwarg please
| def parse_coordinates(coordinates, return_frame=None): | |
| def parse_coordinates(coordinates, *, return_frame=None): |
astroquery/utils/commons.py
Outdated
| except ValueError: | ||
| warnings.warn(f"Failed to transform coordinates to requested frame: {return_frame}.", InputWarning) |
There was a problem hiding this comment.
I feel should let the exception propagate, after all this will only be triggered when return frame is explicitly asked for, thus a problem should be surfaced up to the user/module level
CHANGES.rst
Outdated
| @@ -239,6 +239,8 @@ Infrastructure, Utility and Other Changes and Additions | |||
| - Removed usage of the astropy TestRunner, therefore the unadvertised | |||
| ``astroquery.test()`` functionality. [#3215] | |||
|
|
|||
| - ``return_frame`` parameter in ``utils.commons.parse_coordinates`` returns coordinates in the specified frame. [#3164] | |||
There was a problem hiding this comment.
please rebase to remove the merge commit and to squash out the changelog iterations, and to put this into the 0.4.11 section. Thanks!
snbianco
force-pushed
the
ASB-29990-coord-frame
branch
2 times, most recently
from
42597be to
ef8e8c3
Compare
Fix changelog
Fix changelog
snbianco
force-pushed
the
ASB-29990-coord-frame
branch
from
ef8e8c3 to
43c951c
Compare
|
Updated and rebased! |
This PR handles coordinates passed into query functions that are not in the ICRS frame. If it is found that the given coordinates are not in ICRS, they are transformed.