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

Propagate NaNs and update tests #260

Merged
merged 19 commits into from
Nov 30, 2023
Merged

Propagate NaNs and update tests #260

merged 19 commits into from
Nov 30, 2023

Conversation

richardreeve
Copy link
Contributor

@richardreeve richardreeve commented Nov 29, 2023

This PR will close #259.

@richardreeve
Copy link
Contributor Author

This PR will close #259.

@richardreeve
Copy link
Contributor Author

@devmotion - if you're happy with this I don't know if you want me to bump the package patch version in the PR as well?

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6d0110d) 78.73% compared to head (713cdb5) 95.86%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #260       +/-   ##
===========================================
+ Coverage   78.73%   95.86%   +17.13%     
===========================================
  Files          10       10               
  Lines         602      944      +342     
===========================================
+ Hits          474      905      +431     
+ Misses        128       39       -89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/metrics.jl Outdated Show resolved Hide resolved
src/metrics.jl Outdated Show resolved Hide resolved
src/metrics.jl Outdated Show resolved Hide resolved
@richardreeve
Copy link
Contributor Author

Okay, thanks. I've added back in testing of differing input types for GKL and new testing for KL divergences.

@richardreeve
Copy link
Contributor Author

Okay @devmotion, I've merged your changes (thanks). I was also trying to fix stdlib dependency issues for a new package release and I reinvented part of #258, so I've merged it in including bumping the package version. If you're happy with that, I'm done now (I hope!).

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@richardreeve
Copy link
Contributor Author

Okay, great. It looks like we can merge this and close #258 and #259 then.

@devmotion devmotion merged commit 886ad02 into JuliaStats:master Nov 30, 2023
9 checks passed
@richardreeve richardreeve deleted the rr/kl_fix branch November 30, 2023 16:38
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.

kl_divergence and js_divergence do not propagate NaNs
4 participants