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.

Sign up for GitHub

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

Jump to bottom

Fix standard deviation code in density utils by replacing it with np.std. #1833

Merged
merged 1 commit into from
Oct 2, 2021
Merged

Fix standard deviation code in density utils by replacing it with np.std. #1833

merged 1 commit into from
Oct 2, 2021

Conversation

peterroelants
Copy link
Contributor

@peterroelants peterroelants commented Oct 2, 2021 •
edited by aloctavodia
Loading

Description

Current formula for computing the standard deviation in _kde_linear is not numerically stable. Replace this formula with np.std.

Solves #1832

Checklist

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@peterroelants peterroelants mentioned this pull request Oct 2, 2021
Closed
@peterroelants peterroelants changed the title Use np.std instead of custom code to compute the standard deviation in kde. Use np.std instead of custom code to compute the standard deviation in kde computation. Oct 2, 2021
@peterroelants peterroelants changed the title Use np.std instead of custom code to compute the standard deviation in kde computation. Fix standard deviation code in density utils by replacing it with np.std. Oct 2, 2021
@aloctavodia
Copy link
Contributor

aloctavodia commented Oct 2, 2021 •
edited
Loading

@peterroelants, I just saw your comment on your issue. Thanks for this PR.

@peterroelants @ahartikainen
Use Numpy to compute standard deviation
269a7da 
Current formula for computing the standard deviation in `_kde_linear` is incorrect and could lead to NaNs. Replace with `np.std`.

Co-authored-by: Ari Hartikainen <ahartikainen@users.noreply.github.com>
@peterroelants
Copy link
Contributor Author

It seems like all tests pass. I don't have the rights to merge this myself. Will someone merge this PR if all looks good please?

@aloctavodia aloctavodia merged commit 18d3b3e into arviz-devs:main Oct 2, 2021
@aloctavodia
Copy link
Contributor

Thanks @peterroelants

@peterroelants
Copy link
Contributor Author

Thank you all for the nice library and quick review 👍

@peterroelants peterroelants deleted the patch-1 branch October 4, 2021 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahartikainen ahartikainen ahartikainen approved these changes

@aloctavodia aloctavodia aloctavodia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peterroelants @aloctavodia @ahartikainen