Adding minor tests to test_regtap #427
Merged
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
msdemlei
force-pushed
the
improve-regtap-test-coverage
branch
from
bdbe4b2
to
bdbe74a
Compare
msdemlei
changed the title
msdemlei
force-pushed
the
improve-regtap-test-coverage
branch
from
bdbe74a
to
94a0524
Compare
|
I'd say this should now be ready (and sufficient to close #425); what's uncovered is tedious to cover when weighed against how much it'll actually be used. |
msdemlei
force-pushed
the
improve-regtap-test-coverage
branch
from
94a0524
to
cb425f5
Compare
Codecov Report
@@ Coverage Diff @@
## main #427 +/- ##
==========================================
+ Coverage 79.94% 80.07% +0.13%
==========================================
Files 52 52
Lines 6018 6023 +5
==========================================
+ Hits 4811 4823 +12
+ Misses 1207 1200 -7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
bsipocz
approved these changes
Apr 26, 2023
bsipocz
requested changes
Apr 26, 2023
pyvo/registry/regtap.py
Outdated
| return float(self.get("region_of_regard", 0)) | ||
| # we get NULLs as NaNs here | ||
| val = self["region_of_regard"] | ||
| if val != val: |
There was a problem hiding this comment.
I'm not sure this is the best way to test for np.nans, or what other values would you expect here?
(e.g. np.isnan(val) or np.isfinite(val) is a cleaner check for NaNs)
(in an attempt to tackle astropy#425). This actually uncovered a bug in interpreting region_of_regard; since it now comes out of VOTables, NULL and NaN are the same thing (since nobody looks at region_of_regard, that certainly does not warrant a changelog entry) What is still uncovered is the deprecated ivoid2service, the obscure region_of_regard, and some tedious piece of I/O.
msdemlei
force-pushed
the
improve-regtap-test-coverage
branch
from
cb425f5
to
352ac15
Compare
|
On Wed, Apr 26, 2023 at 11:13:45AM -0700, Brigitta Sipőcz wrote:
@bsipocz requested changes on this pull request.
I'm not sure this is the best way to test for `np.nan`s, or what
other values would you expect here?
Yeah, it's (possibly) NaNs (the universal null value for floats in
VOTables, whether or not we like that). I had wanted to avoid
another import, and x!=x is a rather safe test for "you don't want to
confront normal user code with this" -- but yes, the intent is
clearer with numpy.isnan, so I've force-pushed a change to that
effect.
|
numpy is practically imported at that point with astropy |
bsipocz
approved these changes
Apr 27, 2023
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.
(in an attempt to tackle #425).
closes #425