-
Notifications
You must be signed in to change notification settings - Fork 11
fix(radial): per-object measurement, label-independent (closes #22) #68
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| """Issue #22: get_radial_distribution must give per-object results independent of | ||
| other labels. The default (legacy=False) measures each object on its own crop; | ||
| legacy=True keeps the original whole-image behaviour.""" | ||
|
|
||
| import numpy as np | ||
|
|
||
| from cp_measure.core.measureobjectintensitydistribution import get_radial_distribution | ||
|
|
||
|
|
||
| def _three_touching(): | ||
| """Three abutting rectangles — a layout where the whole-image geometry leaks.""" | ||
| labels = np.zeros((48, 48), dtype=np.int32) | ||
| labels[5:40, 5:18] = 1 | ||
| labels[5:40, 18:31] = 2 | ||
| labels[5:40, 31:44] = 3 | ||
| pixels = np.random.default_rng(1).random((48, 48)) | ||
| return labels, pixels | ||
|
|
||
|
|
||
| def test_radial_per_object_independent_of_neighbours(): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's assume that "legacy" has always covered this (because this is sort-of orthogonal to the legacy implementation since it is due to a dependency) |
||
| """#22: an object's features are identical with or without other labels present. | ||
|
|
||
| Both sides use the per-object default, so this holds bit-exactly even for | ||
| symmetric objects whose centre is a plateau (the crop is the same array).""" | ||
| labels, pixels = _three_touching() | ||
| new = get_radial_distribution(labels, pixels) | ||
| for lbl in (1, 2, 3): | ||
| alone = np.where(labels == lbl, 1, 0).astype(np.int32) | ||
| iso = get_radial_distribution(alone, pixels) | ||
| for key in new: | ||
| np.testing.assert_allclose( | ||
| new[key][lbl - 1], iso[key][0], equal_nan=True, err_msg=key | ||
| ) | ||
|
|
||
|
|
||
| def test_radial_legacy_leaks_where_new_does_not(): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add a note to remove this once github.com/scipy/scipy/pull/25293 is merged.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And maybe the legacy flag for this measurement altogether (?) This one is a tricky one. |
||
| """The whole-image (legacy) path is perturbed by neighbours on this layout while | ||
| the per-object default is not — they differ, proving #22 is real and fixed.""" | ||
| labels, pixels = _three_touching() | ||
| new = get_radial_distribution(labels, pixels) | ||
| legacy = get_radial_distribution(labels, pixels, legacy=True) | ||
| assert any(not np.allclose(new[k], legacy[k], equal_nan=True) for k in new) | ||
|
|
||
|
|
||
| def test_radial_unique_centre_object_new_equals_legacy(): | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is unnecessary, we have already identified the cause of the divergence. Not worth having this test if it will be merged in the future anyways. I'm becoming more keen on raising the lower bound of scipy if it doesn't break other python versions or dependencies. |
||
| """For an unambiguous (odd-sized) centre, the per-object crop reproduces the | ||
| whole-image result exactly — confirming the crop introduces no difference of its | ||
| own; the only systematic divergence is the centre tie-break on symmetric objects.""" | ||
| labels = np.zeros((41, 41), dtype=np.int32) | ||
| labels[8:29, 8:29] = 1 # 21x21 odd square -> unique centre pixel | ||
| pixels = np.random.default_rng(2).random((41, 41)) | ||
| new = get_radial_distribution(labels, pixels) | ||
| legacy = get_radial_distribution(labels, pixels, legacy=True) | ||
| for k in new: | ||
| np.testing.assert_allclose( | ||
| new[k], legacy[k], rtol=1e-6, atol=1e-8, equal_nan=True, err_msg=k | ||
| ) | ||
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.
Turns out things won't be as simple. We should define what "legacy" means:
(3) Cellprofiler before the MAD fix was merged CellProfiler/CellProfiler@02a8b51
(2) scipy before the fix for #22 was merged scipy/scipy#25293
I think "legacy" should mean "before the MAD fix, but not necessarily before the scipy change". It's still unclear how the scipy change will affect the benchmark dataset, but I don't think it will do so significantly. We can add a flag to increase the tolerance for that case /at the test level/.