Repository navigation
fix: derive tile->mask scale from image dimensions - #19
Merged
Merged
Conversation
coverage_stats mapped tile coordinates into mask pixels with `tile_mpp / mask_mpp`, where mask_mpp was the mpp requested in the config rather than the mask's real resolution. Mask builders resolve their target mpp through OpenSlide.closest_level, which snaps to the nearest existing pyramid level: on TCGA slides with downsamples 1/4/16 a 2.0 mpp request lands at ~1.0, so every tile sampled the tissue mask at half its true position. The kept tile set became the tissue footprint magnified 2x about the origin and clipped at the slide edge, which is why whole-slide prediction maps covered glass in places and missed tissue in others while the predictions themselves were correct. The tiling level and each mask depict the same physical slide, so the ratio of their pixel dimensions is the scale. Derive it per mask inside the mask loop (masks in one run can differ in resolution) and per axis, which also fixes anisotropic mpp and absorbs the bad_slides mpp*=2 branch in tissue_masks. config.mpp is no longer used for any computation; it stays in the config as provenance and is logged as a run hyperparameter. Verified against the tissue masks on 6 test slides: IoU between the kept tile footprint and the mask foreground rose from 0.12-0.21 to 0.945-0.976, and per-slide tile counts moved in both directions (+11% to -10%), as a de-skew does rather than a uniform rescale. Existing tissue_stats artifacts are affected; reruns must propagate to the tissue-tile embeddings, which filter on tile_tissue_coverage and are inner-joined at predict time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TwpvonFg4nhkhPFj1EFb9b
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI runs `uvx ruff check`, which pulls the latest ruff (0.16.5), while the pinned dev dependency is `ruff>=0.14.5` and resolves to 0.14.5 locally. The newer version carries ASYNC230, which flags the blocking `open` inside `qc_main` — invisible to local runs, red on GitHub. The rule is right on the merits: the append ran on the event loop while QC results were still streaming in from merge_generators. Move the write into a sync helper dispatched with asyncio.to_thread. It only runs for failed slides, so the thread hop costs nothing on the normal path. Pre-existing on master, unrelated to the coverage_stats fix; surfaced here because the lint workflow runs on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TwpvonFg4nhkhPFj1EFb9b
matejpekar
approved these changes
Sep 18, 2026
vejtek
approved these changes
Sep 25, 2026
vojtech-cifka
added a commit
that referenced
this pull request
Oct 6, 2026
Both artifacts were regenerated after the tile->mask scale fix (#19).
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
3 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.
coverage_stats mapped tile coordinates into mask pixels with
tile_mpp / mask_mpp, where mask_mpp was the mpp requested in the config rather than the mask's real resolution. Mask builders resolve their target mpp through OpenSlide.closest_level, which snaps to the nearest existing pyramid level: on TCGA slides with downsamples 1/4/16 a 2.0 mpp request lands at ~1.0, so every tile sampled the tissue mask at half its true position. The kept tile set became the tissue footprint magnified 2x about the origin and clipped at the slide edge, which is why whole-slide prediction maps covered glass in places and missed tissue in others while the predictions themselves were correct.The tiling level and each mask depict the same physical slide, so the ratio of their pixel dimensions is the scale. Derive it per mask inside the mask loop (masks in one run can differ in resolution) and per axis, which also fixes anisotropic mpp and absorbs the bad_slides mpp*=2 branch in tissue_masks. config.mpp is no longer used for any computation; it stays in the config as provenance and is logged as a run hyperparameter.