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

Quick fix to precision recall #63

Merged
merged 6 commits into from
Oct 20, 2021

Conversation

michaelbornholdt
Copy link
Collaborator

See issue #62 .

@michaelbornholdt michaelbornholdt marked this pull request as ready for review October 14, 2021 19:30
@codecov-commenter
Copy link

Codecov Report

Merging #63 (2f52623) into master (779cea1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #63   +/-   ##
=======================================
  Coverage   98.95%   98.95%           
=======================================
  Files          30       30           
  Lines         956      957    +1     
=======================================
+ Hits          946      947    +1     
  Misses         10       10           
Flag Coverage Δ
unittests 98.95% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cytominer_eval/evaluate.py 100.00% <ø> (ø)
cytominer_eval/operations/precision_recall.py 100.00% <100.00%> (ø)
cytominer_eval/tests/test_evaluate.py 100.00% <100.00%> (ø)
...val/tests/test_operations/test_precision_recall.py 100.00% <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 779cea1...2f52623. Read the comment docs.

@michaelbornholdt
Copy link
Collaborator Author

@gwaygenomics Ready for your review :)

@gwaybio
Copy link
Member

gwaybio commented Oct 15, 2021

@michaelbornholdt - can you resolve the merge conflict?

Also, I feel like @FloHu is better equipped to review this PR than me. @FloHu, given your insights in #62, do you have availability to review within Michael's requested time frame?

@michaelbornholdt
Copy link
Collaborator Author

@FloHu. Everything is ready now.

@FloHu
Copy link

FloHu commented Oct 19, 2021

Sorry for my late answer, things have been very busy. Sure, I'll review it, give me time until tomorrow, it's on my list. :)

Copy link

@FloHu FloHu left a comment

Choose a reason for hiding this comment

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

Everything seems to be correct and making the groupby explicit is definitely needed. I have to say that I still do not see how it solves the issue about counting twice vs. counting once (as discussed in the issue, see last comment) but I trust you on it. When I have some more time I can think into it further.

@michaelbornholdt
Copy link
Collaborator Author

@gwaygenomics please Merge :)

@michaelbornholdt
Copy link
Collaborator Author

michaelbornholdt commented Oct 20, 2021

@FloHu

Everything seems to be correct and making the groupby explicit is definitely needed. I have to say that I still do not see how it solves the issue about counting twice vs. counting once (as discussed in the issue, see last comment) but I trust you on it. When I have some more time I can think into it further.

If you have unique groupings now, ie one group per sample. Then there will no bidirectional vertices since you are only looking at one node. If you group by several nodes, then yes we still have the counting twice thing

@gwaybio gwaybio merged commit 3c907ea into cytomining:master Oct 20, 2021
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.

4 participants