Repository navigation
Multi-slide pooled fit_stain_reference - #1209
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1209 +/- ##
==========================================
+ Coverage 78.84% 78.88% +0.04%
==========================================
Files 65 65
Lines 9591 9611 +20
Branches 1586 1591 +5
==========================================
+ Hits 7562 7582 +20
Misses 1497 1497
Partials 532 532
🚀 New features to boost your workflow:
|
Comment on lines
+345
to
+353
| def test_validation(self, image_key, tissue_mask_key, match: str) -> None: | ||
| sdata, _ = self._cohort(n=2) | ||
| with pytest.raises(ValueError, match=match): | ||
| fit_stain_reference(sdata, image_key, method="macenko", tissue_mask_key=tissue_mask_key) | ||
|
|
||
| def test_list_mask_with_str_image_raises(self) -> None: | ||
| sdata, _ = self._cohort(n=1) | ||
| with pytest.raises(ValueError, match="single `tissue_mask_key`"): | ||
| fit_stain_reference(sdata, "img0", method="macenko", tissue_mask_key=["img0_tissue"]) |
Member
There was a problem hiding this comment.
can these tests be merged into a parameterized one?
Member
Author
There was a problem hiding this comment.
Folded into two parametrized tests: test_pooled_fit (runs + pool-of-one == single, exact except vahadane where NMF is not bit-reproducible even seeded) and test_validation (all raise cases as one table). test_order_matched_non_convention_masks stays separate since it asserts values, not an exception.
Comment on lines
+334
to
+337
| if not image_keys: | ||
| raise ValueError("`image_key` list is empty; pass at least one image key.") | ||
| if len(set(image_keys)) != len(image_keys): | ||
| raise ValueError("`image_key` list has duplicate keys.") |
Member
There was a problem hiding this comment.
Can't we use _unique_order_preserving here somehow?
Member
Author
There was a problem hiding this comment.
Done — duplicate check now goes through _unique_order_preserving.
selmanozleyen
requested changes
Jun 29, 2026
selmanozleyen
left a comment
selmanozleyen
left a comment
Member
There was a problem hiding this comment.
please do the changes I mentioned. Overall writing the code as if all the inputs are [str] is a good idea and normalizing str to [str] will save duplicate computation and code
timtreis
force-pushed
the
stain/pr4-pooled-fit
branch
from
August 27, 2026 15:00
2b517d6 to
3a026a5
Compare
selmanozleyen
left a comment
selmanozleyen
left a comment
Member
There was a problem hiding this comment.
I like to first assess this before going on with the experimental functions: #1279.
It's about the design and the structure. I can look into this PR meanwhile separately but I'd prefer to do after
fit_stain_reference accepts image_key: str | list[str]; a list pools the
tissue pixels of several images into one reference. tissue_mask_key takes
an order-matched list (or None for the {key}_tissue convention). Single
image fit unchanged (a pool of one), so both cases share one path.
timtreis
force-pushed
the
stain/pr4-pooled-fit
branch
from
October 9, 2026 19:36
3a026a5 to
b4a14d9
Compare
- fit_reinhard pools per-channel (count, sum, sum of squares) instead of materialising the tissue pixels, so the Reinhard fit is lazy again and a pooled cohort never holds a frame resident (103 -> 38 MiB peak on a 2000x2000 pool of two). - accumulate those moments in float64. sigma falls out of E[x^2] - E[x]^2, whose terms are ~5000x the variance they bracket; in float32 that cancellation costs ~1% of sigma and makes the answer depend on chunking (eager 0.10272 vs chunked 0.10410 on the same slide). - drop _masked_channel_stats. apply_reinhard was a second implementation of the same reduction and had already diverged from the fit; both now go through _tissue_lab_moments + _stats_from_moments. - raise StainFittingError (with image_key) on empty tissue, matching the decomposition path, instead of a bare ValueError in its own format. - single _resolve_fit_inputs for the coarse image + tissue mask, shared by estimate_white_point, fit_stain_reference and _normalize_stains. - derive the method vocabulary from StainMethod once in _reference.py; drop the second copy in _normalize.py. - document that the pool is a pool of pixels, weighted by tissue pixel count. - tests: assert the error names the slide (the old pattern also matched the message's own trailing "may be blank"), and add pool-of-duplicates invariance, which pool-of-one cannot catch.
luminosity_foreground_mask and absorbance_foreground_mask had no production caller: every fit path composes the conversion and the *_from_lab/_from_sda primitives itself, so the mask reads from the same lazy graph the statistics do, and dask materialises the conversion once. Only tests reached the wrappers. Both conversions already guard the channel dim, so the tests keep their coverage by composing the primitives the way the fits do.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fit_stain_referencenow acceptsimage_key: str | list[str]— a list pools several images' tissue pixels into one fit (one reference for a cohort in one SpatialData).tissue_mask_keytakes an order-matched list (orNonefor the{key}_tissueconvention). Single-image fit unchanged.