-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Subsample checks v2.0! #284
Merged
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
updating branch
Mikejmnez
requested review from
malmans2,
renskegelderloos,
ThomasHaine,
MaceKuailv and
asiddi24
as code owners
December 8, 2022 00:27
Codecov Report
@@ Coverage Diff @@
## main #284 +/- ##
==========================================
- Coverage 95.11% 95.01% -0.11%
==========================================
Files 10 10
Lines 3848 3888 +40
Branches 812 822 +10
==========================================
+ Hits 3660 3694 +34
- Misses 122 125 +3
- Partials 66 69 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Closed
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.
Sorry about the repeated PR.
This PR addresses same issue #281 and has almost identical code as PR #282 .
I found a weird behavior that it never happens with the ECCO and LLC4320 datasets available through SciServer, but happens with the testing ECCO dataset and thus may happen with some other dataset, under the right combination of arguments.
The error is associated with the following line:
where
ARCT[i]
is a list associated with thei-th
face and whose elements are either0
(int) ordataarrays
(_datatype
). When there are zeros (the negative of that if-statement) it means data within the i-th face does not survive the cutout. When the if-statement isTrue
, data within that face must be transformed.The bug:
The
if
statement above assumes the first element (the 0 element) is characteristic of all the elements, but that is not always true. In particular it gives aFalse positive
when the 1st element, a variable or coordinate, doesn't initially haveface
as a dimension (for example thek
, ortime
).This situation happens with the test ECCO dataset but not with the data available through SciServer. Nonetheless, I decided to fix this bug just in case and wrote a test for this scenario.
Everything else is the same as the previous PR.
The solution
The bug is fixed by checking that ALL elements are non-zero (or non-integer). The new line is:
This is the only difference between this PR and PR #282