Add query_regions to esa.hsa#2362
Conversation
|
Hello @keflavich! 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-04-19 16:01:41 UTC |
lvalerom
left a comment
There was a problem hiding this comment.
Looks good to me. I would add a test case for the new function in hsa_test.py:
def test_query_region(self):
c = SkyCoord(ra=100.2417*u.degree, dec=9.895*u.degree, frame='icrs')
parameters = {'coordinate': c,
'radius': 0.5}
dummyTapHandler = DummyHSATapHandler("query_region", parameters)
hsa = HSAClass(self.get_dummy_tap_handler())
hsa.query_region(**parameters)
dummyTapHandler.check_call("query_region", parameters)Co-authored-by: Luis Valero Martín <lvaleromartin@gmail.com>
|
@lvalerom you can submit your suggested change as a PR onto this one. That's the best way to make sure you get credit for those code changes. Your suggestions are all good. |
Codecov Report
@@ Coverage Diff @@
## main #2362 +/- ##
=======================================
Coverage 63.17% 63.17%
=======================================
Files 132 132
Lines 17187 17189 +2
=======================================
+ Hits 10858 10860 +2
Misses 6329 6329
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Add test and documentation to `query_region`
| 7. Query Region | ||
| --------------- | ||
|
|
||
| .. doctest-skip:: |
There was a problem hiding this comment.
I would like to have actual tests for this, either in the doc here, or as remote tests, or both
|
OK, so including the narrative docs we have a decent 84% coverage including the query_region/observations methods, so I go ahead and merge this now. Thank you both! |
The ESA HSA tool should have a
query_regionstool to match it to the general astroquery API. This is my suggestion for how to do that.It also passes on some kwargs that could be useful.