Skip to content
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

fix: wrong calculation for WeightedMean variance NaN's via UHI #695

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Feb 7, 2022

Brought up on Gitter. The old calculation could not have been right, and was not what is described above from the UHI.

@github-actions github-actions bot added the needs changelog Might need a changelog entry label Feb 7, 2022
where=view.sum_of_weights > 1, # type: ignore[union-attr]
where=view.sum_of_weights**2 > view.sum_of_weights_squared,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the... Black, this comment was needed!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue at psf/black#2873, and worked around this by moving this subexpression to a variable, which is a bit more readable, too - but it should not have deleted this comment!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I do plan to eventually make this properly generic, which would fix most or all of these typing issues - probably will be after we drop 3.6, though)

@henryiii henryiii merged commit 61b5c69 into develop Feb 8, 2022
@henryiii henryiii deleted the henryiii/fix/var branch February 8, 2022 03:14
@HDembinski
Copy link
Member

I see you already merged this, so I guess I don't have to check it?

@henryiii
Copy link
Member Author

henryiii commented Feb 8, 2022

You can still check it, it was just a bad bug and the new behavior is at least closer to correct; we can always make a followup PR. Any ideas for further situations to test are always welcome. :)

Starting a discussion (#701) was a good idea.

@henryiii henryiii removed the needs changelog Might need a changelog entry label Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants