Add get_zeropoint and more flexible metadata querying to SVO FPS#3545
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3545 +/- ##
==========================================
+ Coverage 73.23% 73.31% +0.08%
==========================================
Files 226 226
Lines 21010 21038 +28
==========================================
+ Hits 15386 15424 +38
+ Misses 5624 5614 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bsipocz
left a comment
There was a problem hiding this comment.
The changes look good, and tests are happy.
However, I'm less happy about introducing back the **kwarg approach. Could we maybe expand it out and list all the options explicitly? If nothing else, it would make the docstring more useful.
bb82061 to
ac4b698
Compare
ac4b698 to
a4fbe09
Compare
bsipocz
left a comment
There was a problem hiding this comment.
Suggestion for some more docs, and the fix for the RTD failure is in the comments. Also, please rebase to get rid of the merge commit.
127434e to
5168dc2
Compare
|
I think the remaining error here is a temporary service access one from rtd, so this should be good for final review. |
bsipocz
left a comment
There was a problem hiding this comment.
The only actual issue is about testing for the correct exception type we raise, once fixed we should merge this.
bsipocz
left a comment
There was a problem hiding this comment.
Ouch, this is really close, but I think the the parameter cases should be fixed before merging it.
| WavelengthRef_min=None, | ||
| WavelengthRef_max=None, | ||
| WavelengthMean_min=None, | ||
| WavelengthMean_max=None, | ||
| WavelengthEff_min=None, | ||
| WavelengthEff_max=None, | ||
| WavelengthMin_min=None, | ||
| WavelengthMin_max=None, | ||
| WavelengthMax_min=None, | ||
| WavelengthMax_max=None, | ||
| WidthEff_min=None, | ||
| WidthEff_max=None, | ||
| FWHM_min=None, | ||
| FWHM_max=None, | ||
| Instrument=None, | ||
| Facility=None, | ||
| PhotSystem=None, | ||
| ID=None, | ||
| PhotCalID=None, | ||
| FORMAT=None, | ||
| VERB=2, |
There was a problem hiding this comment.
Now that I'm doing a final go through, I noticed that these are cases all over the place. Could you change all of them to be lower case so the basic astroquery API design is not overriden? We already do a mapping, so it shouldn't really affect code complexity
There was a problem hiding this comment.
This matches the documentation on their website (https://svo2.cab.inta-csic.es/theory/fps/index.php?mode=voservice). I'm OK with switching everything to lowercase everywhere if you prefer, but matching the URLs is easier and maybe more intuitive for users?
There was a problem hiding this comment.
I like to think that users would read our documentation and docstrings foremost rather than directly want to match webforms, that's the whole point, right?
(also, I haven't checked, but does the server case sensitive to begin with?)
There was a problem hiding this comment.
it is not case-sensitive! so I will change all to lowercase. I think the underscores are ok but I'll review
There was a problem hiding this comment.
ahhh no I was wrong, it is case sensitive, but it still returns something if you get the case wrong.
bsipocz
left a comment
There was a problem hiding this comment.
OK, this looks good now. I'll go ahead and rebase: 1) smoke out the test failure, that is unrelated but I haven't seen elsewhere 2) get rid of the merge commit 3) squash out the style fixes, including a new commit that fixes the failure
comma, whitespace add tests add missing test data revert an unintended change add doctest remote data tags add changelog entry add get_zeropoint and more flexible metadata querying to SVO FPS comma, whitespace add tests add missing test data revert an unintended change add doctest remote data tags add changelog entry
propagate new keywords around add a correctness check for VERB and FORMAT change order of ops reject kwargs from lowest-level query change the "bad param" test now that we reject kwargs fix unused import Update astroquery/svo_fps/core.py Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com> Update astroquery/svo_fps/core.py Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com> Update astroquery/svo_fps/core.py Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
6541475 to
d4ca72b
Compare
|
RTD failure is unrelated and looks to be temporary server side outage, so I go ahead with the merge here. Thanks @keflavich! |
There is metadata on SVO FPS that is inaccessible but should be.
#3528 showed the right mechanism to retrieve metadata, but it didn't allow querying on PhotCalID - while
data_from_svoallowed specification of this parameter, the mechanism we are using to parse the votable drops the relevant data.This PR adds a new
get_zeropointmechanism to very explicitly and cleanly retrieve zeropoints and also generalizesget_filter_metadata.Note that I have made some serious mistakes in recent publications because I failed to realize the default zeropoint was vega, so it's high importance for me to expose the
get_zeropointmethod with an explicitmag_syskeyword to users.