Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

snbianco
Copy link
Contributor

@snbianco snbianco commented Apr 16, 2025

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 and resolve_all parameters to the resolve_object function. If resolve_all is True, then resolve_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

Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 95.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 69.51%. Comparing base (4ed16e4) to head (08bb761).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/mast/utils.py 94.00% 3 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@snbianco snbianco marked this pull request as ready for review April 17, 2025 21:03
@snbianco snbianco requested a review from bsipocz April 17, 2025 21:03
@bsipocz bsipocz added the mast label Apr 19, 2025
@bsipocz bsipocz added this to the v0.4.11 milestone Apr 19, 2025
@snbianco snbianco marked this pull request as draft April 21, 2025 22:45
@snbianco snbianco marked this pull request as ready for review April 22, 2025 17:15
Copy link
Member

@bsipocz bsipocz left a 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.

@snbianco
Copy link
Contributor Author

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?

Copy link
Member

@bsipocz bsipocz left a 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):
Copy link
Member

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):
Copy link
Member

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

Comment on lines +98 to +99
resolver : str, List, optional
Name of resolver. Must be "NED" or "SIMBAD". This parameter is case-insensitive.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate

Suggested change
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def resolve_object(self, objectname, resolver=None, resolve_all=False):
def resolve_object(self, objectname, *, resolver=None, resolve_all=False):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: MAST TypeErrors for fill values
2 participants