[bugfix] WeightedVariance is broken in a couple of ways in yt-4.0 #2348
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
|
Ugh, sorry for the pain my typo back in March caused you. Thank you for tracking this down and fixing it. |
|
@ngoldbaum no big deal, someone will catch something I did next, I am sure. |
munkm
approved these changes
Oct 25, 2019
|
Ok @jzuhone the fix for the Travis tests should be on the 4.0 branch now. If you merge or rebase those tests should pass. 🙂 |
have no data (this runs into a units problem).
|
I'm satisfied with the test coverage here, actually. This is ok to merge whenever. |
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 the
yt-4.0branch, theWeightedVariancederived quantity (and thus thestdmethod of data containers which is downstream from it) has a couple of bugs.If one runs into a case where some chunks have a weight which sums to zero, the code returns floating-point zeros to be used in the final variance calculation. However, the chunks with data are not dimensionless in general, and so when one attempts to add everything together you get a
IterableUnitCoercionError. This PR fixes that by dropping the units during the calculating and assigning them at the end.I also noticed after fixing 1. that the answers given were not the same as if one had simply computed the weighted variance by hand using NumPy on the data object's field. I tracked this second error down to commit 191056c.
Which was just a mix-up in the reformatting of the line.
I'll see what I can do about improving test coverage for this.