Simplify simbad code#2494
Conversation
The wildcards allowed by `Simbad` are stored in a class constant together with their descriptions. A separate constant that stored the wildcards without the descriptions but in order had become unneccessary because starting from Python 3.7 `dict` keys retain insertion order.
The local Boolean variable `vector` is checked in a few if- and elif-statements, but it is guaranteed to be `True` and can therefore be removed.
The removed function duplicated the functionality `astropy.utils.isiterable()`.
The single use of the `copy` module has been replaced with calling `list.copy()`.
`SimbadBibcodeResult.table` and `SimbadObjectIDsResult.table` have been simplified. Most importantly they no longer construct the required `Table` row-by-row because adding rows to a `Table` is expensive.
Various small simplifications throughout the module.
Codecov Report
@@ Coverage Diff @@
## main #2494 +/- ##
==========================================
- Coverage 62.90% 62.86% -0.05%
==========================================
Files 133 133
Lines 17308 17276 -32
==========================================
- Hits 10888 10860 -28
+ Misses 6420 6416 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bsipocz
left a comment
There was a problem hiding this comment.
There is is a bug in one of the scenarios (vector coords + string scalar radius), which is also a bug in the current main, but the refactoring should better fix it while rewriting the code.
| print("{key} : {value}\n".format(key=key, | ||
| value=self.WILDCARDS[key])) | ||
| return | ||
| print("\n\n".join(f"{k} : {v}" for k, v in self.WILDCARDS.items())) |
There was a problem hiding this comment.
it's ok to keep it like this, but overall I value non-single-letter variables even when the intention is clear like in this line. (comment is for here and also for cases below)
| elif vector and _has_length(radius) and len(radius) != len(ra): | ||
| raise ValueError("Mismatch between radii and coordinates") | ||
| elif vector and not _has_length(radius): | ||
| if isiterable(radius): |
There was a problem hiding this comment.
as I see at this point radius can still be a string, this would add a bug for a single string radius for a list of RA case. Maybe also add a test for that case? (as I see vector queries are totally missing from the local tests and for multicord tests only quantity scalar radius is being used for the remote tests)
|
Changelog entry will be needed for the bugfix part. |
The new tests reveal a problem when radius is passed as a `str` and multiple coordinates are specified.
It is now possible to specify multiple coordinates together with a single radius as a `str`.
|
I have fixed the bug and improved test coverage. |
The new test asserts that the expected `ValueError` is raised if multiple coordinates are specified together with an incompatible number of radii.
cf128c1 to
35b13ec
Compare
|
Thanks, this is all good now! |
I have applied various small simplifications throughout
astroquery/simbad/core.py. See the commit messages for details.