Simplify simbad code
#2494
Merged
Simplify simbad code
#2494
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
requested changes
Aug 17, 2022
| 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)
astroquery/simbad/core.py
Outdated
| 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.
eerovaher
force-pushed
the
simbad-refractor
branch
from
cf128c1 to
35b13ec
Compare
bsipocz
approved these changes
Aug 17, 2022
|
Thanks, this is all good now! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have applied various small simplifications throughout
astroquery/simbad/core.py. See the commit messages for details.