Bugfix: SIMBAD test cleanup, ROW_LIMIT support #2637
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2637 +/- ##
==========================================
+ Coverage 68.90% 68.94% +0.03%
==========================================
Files 304 304
Lines 22621 22621
==========================================
+ Hits 15587 15596 +9
+ Misses 7034 7025 -9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
astroquery/exceptions.py
Outdated
| @@ -111,3 +112,11 @@ class EmptyResponseError(ValueError): | |||
| Astroquery error class to be raised when the query returns an empty result | |||
| """ | |||
| pass | |||
|
|
|||
|
|
|||
| class BadRowWarning(AstropyWarning): | |||
There was a problem hiding this comment.
I'm not so sure about the naming of this, is this a row, or an entry, or an object? after all this warning is expected with e.g. vectorized query_object queries. (what is actually/orginal error/warning coming back? (I suppose I can check on it tomorrow myself)
There was a problem hiding this comment.
The error can vary. For example, in one of the tests, it is:
[5] 'gHer' this identifier has an incorrect format for catalogs:
gH : Gurwell + Hodge
g : Giclas
In another case, it is:
[5] 'dosntexist': No known catalog could be found
But the net effect is to not include that search term in the output results.
Maybe this should be MissingRowWarning or BlankResponseWarning? I think it is right that we raise a warning rather than an error, because it is often valuable to run a query where you do not expect results for all search terms.
There was a problem hiding this comment.
Still, my problem with the "Row" remains, it's feels like not the right word. MissingObjectWarning/MissingTargetWarning/BadObjectWarning, or in fact your suggestion of BlankResponseWarning could all work better.
(it's an implementation detail that the result will be returned one object per row, it could be a different structure, too, so I think the warning shouldn't really refer to this detail)
There was a problem hiding this comment.
Yeah sounds good - I'll change to... I guess BlankResponseWarning, then? It's not necessarily a missing object, since the first example says 'there might be an object there but you got the name format wrong'.
|
This will need a changelog for sure. |
keflavich
changed the title
Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
Co-authored-by: Eero Vaher <eero.vaher@gmail.com>
|
Thanks @keflavich! |
|
Ohh, we should have squash down the bit of the back and forth before merging, well, next time. |
|
sorry yeah, I meant to do a squash before the end |
This error has been present since day 1:
https://github.com/astropy/astroquery/blame/4822f81d66ed5322d2c3b6e23a51c3595545bc1b/astroquery/simbad/tests/test_simbad_remote.py#L16
I'm not clear why this wasn't causing errors before.
This is a partial fix for some open remote test failures in #2634, but is a WIP as I expect there are others to fix.