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

fixed ARI (fixes #225 & #226) #227

Merged
merged 4 commits into from
Mar 19, 2023
Merged

fixed ARI (fixes #225 & #226) #227

merged 4 commits into from
Mar 19, 2023

Conversation

wildart
Copy link
Contributor

@wildart wildart commented Dec 25, 2021

  • added pair confusion matrix (CM) calculations
  • fixed ARI calculation using CM

@codecov-commenter
Copy link

codecov-commenter commented Dec 25, 2021

Codecov Report

Base: 95.18% // Head: 95.12% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (fc57919) compared to base (82821e8).
Patch coverage: 90.47% of modified lines in pull request are covered.

❗ Current head fc57919 differs from pull request most recent head 945c537. Consider uploading reports for the commit 945c537 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage   95.18%   95.12%   -0.06%     
==========================================
  Files          16       17       +1     
  Lines        1328     1333       +5     
==========================================
+ Hits         1264     1268       +4     
- Misses         64       65       +1     
Impacted Files Coverage Δ
src/confusion.jl 83.33% <83.33%> (ø)
src/randindex.jl 100.00% <100.00%> (+5.55%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wildart
Copy link
Contributor Author

wildart commented Dec 25, 2021

As rand behavior changed in recent julia versions, the rework of RNG handling or a test review is required.

@pcjentsch
Copy link

This PR passes the relevant tests, can it be merged?

This is an old bug that has had other PRs to fix it: #99

@ararslan ararslan closed this Sep 10, 2022
@ararslan ararslan reopened this Sep 10, 2022
@ararslan
Copy link
Member

Looks like tests are passing except for 32-bit Linux. I wonder if the forced conversion to Int in confusion means that something in randindex overflows when Int is only half as big.

test/randindex.jl Outdated Show resolved Hide resolved
- added pair confusion matrix (CM) calculations
- fixed ARI calculation using CM
@pcjentsch
Copy link

pcjentsch commented Sep 23, 2022

Hm, not sure why that change broke the test. Sorry about that, I did test it locally before suggesting it and it was fine.

Copy link
Member

@alyst alyst left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Looks good. Could you please implement the suggestions (which are basically just formatting improvements)?

doc/source/validate.md Outdated Show resolved Hide resolved
doc/source/validate.md Outdated Show resolved Hide resolved
src/confusion.jl Outdated Show resolved Hide resolved
src/confusion.jl Outdated Show resolved Hide resolved
src/confusion.jl Outdated Show resolved Hide resolved
src/confusion.jl Show resolved Hide resolved
src/confusion.jl Outdated Show resolved Hide resolved
src/randindex.jl Outdated Show resolved Hide resolved
src/randindex.jl Outdated Show resolved Hide resolved
test/randindex.jl Outdated Show resolved Hide resolved
@alyst
Copy link
Member

alyst commented Mar 19, 2023

@wildart So sorry I overlooked your updates! Thank you!

alyst added a commit that referenced this pull request Mar 20, 2023
for very large clusterings the agreement/disagreement counts are
very large, so we have to switch to float when multiplying them

fixes #225
enhances #227
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.

5 participants