Get IBE's query_region_async function minimally working#1430
Get IBE's query_region_async function minimally working#1430odysseus9672 wants to merge 12 commits intoastropy:mainfrom
Conversation
Resubmitting bug fix I accidentally deleted.
|
Hello @odysseus9672! 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 2019-04-26 02:14:32 UTC |
keflavich
left a comment
keflavich
left a comment
There was a problem hiding this comment.
If there are big chunks of the code that don't do anything, it's better to remove them than comment that they do nothing.
|
|
||
| # Raise exception, if request failed | ||
| response.raise_for_status() | ||
| HTMLtoTable = HTML() |
There was a problem hiding this comment.
It would be nicer to use the Table initializer directly
astroquery/ibe/core.py
Outdated
| TIMEOUT = conf.timeout | ||
|
|
||
| def query_region( | ||
| self, coordinate=None, where=None, mission=None, dataset=None, |
There was a problem hiding this comment.
Removing kwargs, even if they are unused, should be done via a deprecation.
There is a decorator for doing that in astropy, and it was backported to the LTS release (2.0.12), too, but not to 3.0.x, so I suppose we shouldn't yet use it here.
@keflavich - what do you think? In astroML I worked around this very same issue by copying the decorator to astroML.utils and importing it from there when the astropy version is not the preferred one (we also do similar things in photutils to work around issues that are not available in all supported versions of astropy).
There was a problem hiding this comment.
I'm onboard with using those decorators, but in this case, I think these kwargs are being removed because they didn't do anything? But your point below that it's breaking the tests suggests that yes, we should use the deprecation and copy over the deprecator decorator.
|
The test failures are related to the fact of some of the kwargs got removed. This may bite back where users are using those kwargs as positional arguments, too.
|
|
I had a quick look into this, and it seems that the issues are deeper than one could fix them with the decorators. E.g.: some functionality are broken after this PR that are still working on master. vs a HTTPError on this PR. Which shows that our test suite is quite broken and incomplete. Thus I would suggest a test driven approach, e.g., add new tests run remotely for each method (similar to the ones in the local ones), and in the first approximation make sure that they keep working and return some sensible results both before and after adding changes. |
|
I started another approach to fixing this when I was trying to get IBE to work: keflavich@c5efe04. It's redundant with this PR, and I never got it to work (IBE's structure fundamentally confuses me still), but the alternate approaches explored there may be helpful. |
4 participants
This pull request gets astroquery's IBE module back into a minimally functional state. It includes the fixes in pull requests #1426 and #1428 , so addresses issues #1423 and #1427. It also addresses issue #1429. The SIA functionality is outright disabled, because it uses a different interface on IRSA's website. Data downloading is also not addressed by this pull request.