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.

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

fix: count_ratio on non overlapping partitions #886

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+525 −27
Open

fix: count_ratio on non overlapping partitions #886

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2405b8e
fix: count_ratio on non overlapping partitions
drodarie Sep 21, 2024
bc8e9e5
fix: parallel array placement with offset #889
drodarie Oct 1, 2024
6f0cc5b
docs: add first round of documentation on Placement and Indicators ba…
drodarie Oct 1, 2024
12bd416
fix: re implement the original count_ratio strategy as a new local one.
drodarie Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions bsb/placement/indicator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,10 @@ def guess(self, chunk=None, voxels=None):
relation = relative_to
if count_ratio is not None:
strats = self._strat.scaffold.get_placement_of(relation)
estimate = (
sum(
PlacementIndicator(s, relation).guess(chunk, voxels)
for s in strats
)
* count_ratio
estimate = self._estim_for_chunk(
chunk,
sum(PlacementIndicator(s, relation).guess() for s in strats)
* count_ratio,
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

@Helveg Helveg Sep 29, 2024 •
edited
Loading

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.

Copy link
Contributor

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 🤗

)
elif density_ratio is not None:
# Create an indicator based on this strategy for the related CT.
Expand Down
20 changes: 20 additions & 0 deletions tests/test_placement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
PlacementError,
PlacementRelationError,
PlacementStrategy,
Rhomboid,
Scaffold,
VoxelData,
Voxels,
Expand Down Expand Up @@ -222,6 +223,25 @@ def test_guess_vox_density(self):
# voxel density key not found
dud4_ind.guess(voxels=self.voxels.get_voxelset())

def test_regression_issue_885(self):
# Test placement with count ratio in separated partitions
self.network.topology.children.append(
part := Rhomboid(
name="dud_layer2", origin=[0, 0, 120], dimensions=[200, 200, 80]
)
)
self.network.resize()
placement = PlacementDud(
name="dud",
strategy="PlacementDud",
partitions=[part],
cell_types=[self.network.cell_types["cell_rel_count"]],
)
self.network.placement["dud3"] = placement
indic = placement.get_indicators()["cell_rel_count"]
self.assertEqual(20 / 4, indic.guess(_chunk(0, 0, 1)))
self.assertEqual(20 / 4, indic.guess(_chunk(1, 1, 1)))

def test_negative_guess_count(self):
self.placement = single_layer_placement(
self.network, offset=np.array([-300.0, -300.0, -300.0])
Expand Down
Loading