search

brainglobe / brainglobe-utils

Shared general purpose tools for the BrainGlobe project
MIT License
11 stars 1 forks source link

[BUG] Cell < comparison #60

Closed K-Meech closed 5 months ago

K-Meech commented 5 months ago

Describe the bug The current implementation of < for cells checks the z, y and x coordinate in order. This seems odd as it means that, for example, a cell with [x, y, z] position [100, 100, 4] would be classed as less than one with [-100, -100, 5] even though only the z coordinate is lower. Should it be checking that all coordinates (x, y and z) are lower than the other cell, rather than one at a time?

To Reproduce

from brainglobe_utils.cells.cells import Cell

print(Cell([100, 100, 4], Cell.CELL) < Cell([-100, -100, 5], Cell.CELL))

Expected behaviour I'd expect the above to return False, and only cases where all coordinates are lower to return True e.g.

from brainglobe_utils.cells.cells import Cell

print(Cell([4, 5, 6], Cell.CELL) < Cell([5, 6, 7], Cell.CELL))
adamltyson commented 5 months ago

TBH I have no idea if or how this is even used. It's likely it can just be removed.

K-Meech commented 5 months ago

Ok - thanks. I'll remove it in the PR I'm currently working on.

adamltyson commented 5 months ago

It would be worth checking for usage, just in case.

K-Meech commented 5 months ago

It seems this is used in at least one place in BrainGlobe - e.g. it gets called via natsorted which is run via various tests. It's difficult to find all usages of this, as it will be called whenever < is used with two cells, even inside functions like natsorted.

I think it's best to leave this as-is for now, to avoid breaking any existing functionality. I'll close this issue - but feel free to re-open if you'd prefer it to be removed/changed.