Skip to content

Conversation

@j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented Mar 7, 2023

This PR addresses the fact that Cov3d clustering indices maybe permuted from the Simulation volume indices or states, mentioned in #823

@j-c-c j-c-c added the bug Something isn't working label Mar 7, 2023
@j-c-c j-c-c self-assigned this Mar 7, 2023
@j-c-c j-c-c requested a review from garrettwrong March 7, 2023 16:51
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #873 (1681892) into develop (b60c0ee) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 1681892 differs from pull request most recent head 15e5a18. Consider uploading reports for the commit 15e5a18 to get more accurate results

@@           Coverage Diff            @@
##           develop     #873   +/-   ##
========================================
  Coverage    88.68%   88.68%           
========================================
  Files          116      116           
  Lines         9439     9439           
========================================
  Hits          8371     8371           
  Misses        1068     1068           
Impacted Files Coverage Δ
src/aspire/source/simulation.py 99.44% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

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

This problem is famous. For a statistic see adjusted rand index.

For similar loosely related algorithms, see both Edit Distance family, the Hungarian Problem family, and bipartite matching. Although I think the statistic I mention is the exact thing to use if we are trying to solve this for the general case.

Let me know if the statistic will cover the issue, thanks.

@j-c-c j-c-c requested a review from garrettwrong March 8, 2023 19:33
garrettwrong
garrettwrong previously approved these changes Mar 10, 2023
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

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

Code lgtm, thanks.

Can you squash this down to one commit? While doing that, you can add the Co-authored-by: on the commit so I get that elusive Pair Extraordinaire badge 😇 .

@j-c-c
Copy link
Collaborator Author

j-c-c commented Mar 10, 2023

Code lgtm, thanks.

Can you squash this down to one commit? While doing that, you can add the Co-authored-by: on the commit so I get that elusive Pair Extraordinaire badge 😇 .

Will do! Sorry forgot to add you to that!

Co-authored-by: Garrett Wright <47759732+garrettwrong@users.noreply.github.com>
@j-c-c j-c-c force-pushed the eval_coords_823 branch from 1681892 to 15e5a18 Compare March 10, 2023 16:18
@j-c-c j-c-c marked this pull request as ready for review March 10, 2023 16:23
@j-c-c j-c-c requested a review from janden as a code owner March 10, 2023 16:23
@garrettwrong
Copy link
Collaborator

I think we can spare Joakim on reviewing this one if you want to merge it in (we talked about it yesterday). Ty!

@j-c-c j-c-c merged commit 8ec2b26 into develop Mar 10, 2023
@j-c-c j-c-c deleted the eval_coords_823 branch March 10, 2023 18:29
@j-c-c j-c-c mentioned this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants