-
-
Notifications
You must be signed in to change notification settings - Fork 413
Pass in resolver name to MAST query functions #3292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3292 +/- ##
==========================================
+ Coverage 69.36% 69.51% +0.15%
==========================================
Files 232 232
Lines 19692 19739 +47
==========================================
+ Hits 13659 13722 +63
+ Misses 6033 6017 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a related test failure, please have a look into it:
>>> ned_loc = utils.resolve_object("jw100", resolver="NED")
...
E astroquery.exceptions.ResolverError: Could not resolve jw100 to a sky position using NED. Please try another resolver or set ``resolver=None`` to use the first compatible resolver.
Hmm, I can't reproduce this. I believe that changes were just deployed to our SANTA service, so you may have caught it while it was down? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments about mandatory kwargs. Besides those this needs a rebase to resolve the changelog conflict
@@ -90,39 +87,115 @@ def _simple_request(url, params=None): | |||
return response | |||
|
|||
|
|||
def resolve_object(objectname): | |||
def resolve_object(objectname, resolver=None, resolve_all=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these mandatory kwargs, here and the other methods, too.
return coord | ||
|
||
|
||
def parse_input_location(coordinates=None, objectname=None, resolver=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make all of these mandatory kwargs, too. I know it's breaking API, but it may prevent some buggy user code in the longer run
resolver : str, List, optional | ||
Name of resolver. Must be "NED" or "SIMBAD". This parameter is case-insensitive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate
resolver : str, List, optional | |
Name of resolver. Must be "NED" or "SIMBAD". This parameter is case-insensitive. |
@@ -110,18 +110,29 @@ def disable_cloud_dataset(self): | |||
""" | |||
pass | |||
|
|||
def resolve_object(self, objectname): | |||
def resolve_object(self, objectname, resolver=None, resolve_all=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def resolve_object(self, objectname, resolver=None, resolve_all=False): | |
def resolve_object(self, objectname, *, resolver=None, resolve_all=False): |
This PR adds a
resolver
parameter to various MAST query functions. This parameter allows users to specify what resolver they want to use when resolving object names into coordinates.It also adds
resolver
andresolve_all
parameters to theresolve_object
function. Ifresolve_all
isTrue
, thenresolve_object
will return a dictionary where keys are resolver names and values are their resolved coordinates.I added test cases and updated our documentation to reflect these additions.
Also fixes some test failures brought on by numpy 2.0 changes and closes #3299