Hubble related members #2268
Hubble related members #2268
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2268 +/- ##
==========================================
+ Coverage 63.08% 63.09% +0.01%
==========================================
Files 131 131
Lines 17005 17037 +32
==========================================
+ Hits 10728 10750 +22
- Misses 6277 6287 +10
Continue to review full report at Codecov.
|
|
Dear Astroquery team, Please let me know if there are any changes you see are necessary for the acceptance of the pull request. Many thanks, Javier B |
There was a problem hiding this comment.
There are some public-facing functions that could use docstrings, otherwise this is fine.
I'll note that we have an open issue here we should correct - async in the astroquery API is supposed to be a separate function, not something specified as a keyword argument. That doesn't need to be fixed here, but we'd like the interface to be consistent.
There was a problem hiding this comment.
There are some duplicated commits, as well as a minor changelog conflict. Could you please rebase? (Interactive rebase is needed for removing the duplicates, if that's the only remaining issue I'm happy to do it for this PR).
My comments are mainly minor things and opportunities for some code cleanups. There is one API change that is somewhat critical to be addressed before merging this.
Updated CHANGES.rst
javier-ballester
force-pushed
the
hubble_related_members
branch
from
5eeff2b to
804a2c2
Compare
|
Dear @bsipocz, Thank you for all the corrections. I have tried to solve all of them but let me know if anything is missing. Many thanks, Javier B |
astroquery/esa/hubble/core.py
Outdated
| @@ -380,7 +465,11 @@ def cone_search_criteria(self, radius, target=None, | |||
| raise TypeError("Please use only target or coordinates as" | |||
| "parameter.") | |||
| if target: | |||
| ra, dec = self._query_tap_target(target) | |||
| coord = self._query_tap_target(target) | |||
| ra = coord.get('RA_DEGREES') | |||
There was a problem hiding this comment.
The get() lines won't work with SkyCoords (as I see you indeed changed _query_tap_target() to return a skycoord). So this whole conditional could be simplified a little.
There was a problem hiding this comment.
Please add a test that tests this use case, so CI can pick it up, too.
In [4]: ESAHubble.cone_search_criteria(target='M11', radius=1)
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
<ipython-input-4-be2eb514e1e9> in <module>
----> 1 ESAHubble.cone_search_criteria(target='M11', radius=1)
~/munka/devel/worktrees/astroquery/testings/astroquery/esa/hubble/core.py in cone_search_criteria(self, radius, target, coordinates, calibration_level, data_product_type, intent, obs_collection, instrument_name, filters, async_job, filename, output_format, save, cache, verbose)
467 if target:
468 coord = self._query_tap_target(target)
--> 469 ra = coord.get('RA_DEGREES')
470 dec = coord.get('DEC_DEGREES')
471
~/.pyenv/versions/3.9.1/lib/python3.9/site-packages/astropy/coordinates/sky_coordinate.py in __getattr__(self, attr)
856
857 # Fail
--> 858 raise AttributeError("'{}' object has no attribute '{}'"
859 .format(self.__class__.__name__, attr))
860
AttributeError: 'SkyCoord' object has no attribute 'get'
|
Hi @bsipocz, I have added the changes you have requested and have added additional unit tests to cone_search_criteria to cover the missing test cases. Many thanks, Javier |
|
Thank you @javier-ballester! |
These changes allow users to get related members of HST and HAP observations. Query_target method has been refactored to use the TAP instead of AIO.