-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: count_ratio on non overlapping partitions #886
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 87.89% 88.97% +1.08%
==========================================
Files 103 110 +7
Lines 13771 15141 +1370
==========================================
+ Hits 12104 13472 +1368
- Misses 1667 1669 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution introduces breaking changes: the current implementation of count_ratio
intentionally follows the local density of the related cell type. The break probably went unnoticed because tests for this feature are missing?
estimate = self._estim_for_chunk( | ||
chunk, | ||
sum(PlacementIndicator(s, relation).guess() for s in strats) | ||
* count_ratio, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think the previous version counted the related cell in the analogous chunk & voxelset intersect, and then multiplied the ratio. It seems your new code:
- Takes the global
relation_count * ratio
- Then estimates the own count for the chunk
Is this strictly better? It seems like both might be valid use cases? Sometimes a user may want the density/count to be analogous to the cell type in the same chunk/voxels? Maybe it's better to deprecate count_ratio
and introduce 2 new indication methods local_count_ratio
and global_count_ratio
?
We'd probably have to do the same for some of the density ratio methods then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My implementation is not better but IMHO more intuitive. For instance, let' s say you want to place 1 IO cell for every 2 PC, here these two cells would not be in overlapping partitions.
This previous use case (A) is legit but very specific, I think most users would use the second one (B). Hence my suggestion as a "fix".
From the user point of view the two strategies are very close so I would suggest to give B an additional boolean flag "intersect" so that we could keep A?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, since we have no spec or definition, or tests, we can consider your enhancement a "bugfix", in return, could you write docs about each of these indicator methods in this PR so that the behaviour is defined? :)
I think it would also be minimal effort to keep the existing logic in a new indicator method "local_count_ratio"
? But it's not a hill I would die on if you just want to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you provide the text I will provide images 🤗
Describe the work done
Make sure the volume ratio computation depends on the relative cell instead of the target cell.
List which issues this resolves:
fix #885, #889
Tasks
📚 Documentation preview 📚: https://bsb--886.org.readthedocs.build/en/886/