-
-
Notifications
You must be signed in to change notification settings - Fork 397
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
adding support for calibration queries #2288
base: main
Are you sure you want to change the base?
Conversation
…ab equivalent from the website)
Hello @olyoberdorf! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-08-18 00:45:34 UTC |
Using any of the already required dependencies is fair game and encouraged (https://astroquery.readthedocs.io/en/latest/#requirements). If for some reason a supported version needs to be bumped to a newer minimal, that could very much be OK, but needs discussion (we tend not to drop support unless it comes up as a blocker). The least preferable thing, adding any new dependencies should certainly need a prior discussion as in many times it may be possible to work around with the current dependencies. |
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.
Overall this looks good to me, except the test failures are real as some corner cases should be treated in the code. I'll come back for a more thorough review once the CI is all happy.
astroquery/gemini/core.py
Outdated
# This is future-proofing, the existing page has a bug putting the header inside the body | ||
# but if that gets fixed, we'll want this | ||
cols = None | ||
table_header = table.find("thead") |
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.
This has to handle NoneType
as an option for table
, at least the test failure seems legit to me.
Adding some narrative docs would be welcome, too. Please note though that some changes to the gemini docs was merged very recently (now we test the docs examples), so this would need a rebase to pick up those changes. And I forgot to mention the changelog, please add an entry there, too. Thanks! |
…ab equivalent from the website)
…troquery into gemini_calibration_search
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.
going through the code I wonder whether these should be separate methods, or rather a new kwarg for the existing query_region
, query_object
etc.
Also, test failures are very much related and has to be fixed before a proper round of review.
astroquery/utils/commons.py
Outdated
@@ -23,7 +23,7 @@ | |||
from astropy.coordinates import BaseCoordinateFrame | |||
|
|||
from ..exceptions import TimeoutError, InputWarning | |||
from .. import version | |||
from .. import __version__ as version |
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.
please remove this change
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.
oops, done
return self.query_calibrations_for_criteria(objectname=objectname, radius=radius) | ||
|
||
@class_or_instance | ||
def query_calibrations_for_criteria(self, *rawqueryargs, coordinates=None, radius=None, pi_name=None, program_id=None, utc_date=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.
what are the possible rawqueryargs and rawquerykwargs possibilities? We preferably should not have those wildcard type options.
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.
The calls are really mapping to a URL search on the Gemini archive website. These consist of an arbitrary set of arguments in the URL path or key=value arguments as well. It's order independent on the website.
In the case of query_raw
and query_calibrations_raw
I really prefer this open-ended approach so the API mirrors what the web API is doing. For the _criteria
calls I could drop them. They are just a convenience if you want to use the more well defined functions but still slip in one or two unusual criteria or terms.
This is an API equivalent to the "associated calibrations" tab on the Gemini Archive website. It uses comparable searches to the observation data and returns results one would see on that tab.
query_calibrations_for_region - This allows you to get calibrations against a cone search at some coordinate
query_calibrations_for_object - This gets calibrations related to a search by object name
query_calibrations_for_criteria - This allows for a richer query with multiple terms and returns the calibrations
query_calibrations_raw - For expert use, this allows more free-form search terms based on a URL from the website
Note this pull request depends on BeautifulSoup. It's already an astropy dependency, but I wasn't sure if there's a desire to avoid it.