Skip to content

Do not return 0 if metric value is nan in correlation function #588

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
Merged
mulhod merged 4 commits into from
Mar 18, 2020
Merged

Do not return 0 if metric value is nan in correlation function #588

mulhod merged 4 commits into from
Mar 18, 2020

Conversation

@mulhod
Copy link
Contributor

@mulhod mulhod commented Oct 28, 2019

Addresses #585.

@mulhod
Copy link
Contributor Author

mulhod commented Oct 28, 2019

This will have to wait until #586 is finished.

@mulhod mulhod changed the title Do not return 0 if metric value is nan in correlation function [WIP] Do not return 0 if metric value is nan in correlation function Oct 28, 2019
@mulhod mulhod force-pushed the bugfix/585_correlation branch from 76f2303 to d74f616 Compare October 30, 2019 17:18
@codecov
Copy link

codecov bot commented Oct 30, 2019 •
edited
Loading

Codecov Report

Merging #588 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #588   +/-   ##
=======================================
  Coverage   95.06%   95.06%           
=======================================
  Files          20       20           
  Lines        2977     2977           
=======================================
  Hits         2830     2830           
  Misses        147      147
Impacted Files Coverage Δ
skll/metrics.py 97.14% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f7a6fa...4d2a9a8. Read the comment docs.

@mulhod mulhod changed the title [WIP] Do not return 0 if metric value is nan in correlation function Do not return 0 if metric value is nan in correlation function Oct 30, 2019
desilinguist
desilinguist approved these changes Oct 30, 2019
View reviewed changes
Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mulhod
Copy link
Contributor Author

mulhod commented Oct 30, 2019

This is ready for review now.

@mulhod
Copy link
Contributor Author

mulhod commented Mar 17, 2020

Does someone else have time to review this? @aoifecahill @bndgyawali @ananyaganesh @AVajpayeeJr @chaomenghsuan

@desilinguist
Copy link
Collaborator

desilinguist commented Mar 17, 2020

@mulhod I merged master into this just to double check that nothing breaks with the new scikit-learn.

ananyaganesh
ananyaganesh approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@ananyaganesh ananyaganesh left a comment

Choose a reason for hiding this comment

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

LGTM!

@mulhod
Fix test_send_warnings_to_log, which had formerly been expecting to c…
1f9d491 
…heck for a metric warning from scikit-learn, which is not generated now at all due to the bugfix; remove the test skipping for Windows
@mulhod mulhod changed the title Do not return 0 if metric value is nan in correlation function [WIP] Do not return 0 if metric value is nan in correlation function Mar 18, 2020
@mulhod
Copy link
Contributor Author

mulhod commented Mar 18, 2020

Even though it says this passed all checks, there was one failure that still needs to be investigated. I'm not sure why it says all checks passed.

@mulhod mulhod changed the title [WIP] Do not return 0 if metric value is nan in correlation function Do not return 0 if metric value is nan in correlation function Mar 18, 2020
@mulhod
Copy link
Contributor Author

mulhod commented Mar 18, 2020

This is good to go now.

@mulhod mulhod merged commit 1d64e25 into master Mar 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/585_correlation branch March 18, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@desilinguist desilinguist desilinguist approved these changes

@aoifecahill aoifecahill Awaiting requested review from aoifecahill

@AVajpayeeJr AVajpayeeJr Awaiting requested review from AVajpayeeJr

@chaomenghsuan chaomenghsuan Awaiting requested review from chaomenghsuan

+2 more reviewers

@ananyaganesh ananyaganesh ananyaganesh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mulhod @desilinguist @ananyaganesh