-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Feature added: Get WSI at mpp #7574
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: dev
Are you sure you want to change the base?
Feature added: Get WSI at mpp #7574
Conversation
…izing function to Image.resize, cucim.skimage.transform.resize
0b7322e to
1c10112
Compare
…roject-MONAI#7308) ### Description Based on the discussion topic [here](Project-MONAI#7161 (comment)), we implemented the Conjugate-Gradient algorithm for linear operator inversion, and Stein's Unbiased Risk Estimator (SURE) [1] loss for ground-truth-date free diffusion process guidance that is proposed in [2] and illustrated in the algorithm below: <img width="650" alt="Screenshot 2023-12-10 at 10 19 25 PM" src="https://github.com/Project-MONAI/MONAI/assets/8581162/97069466-cbaf-44e0-b7a7-ae9deb8fd7f2"> The Conjugate-Gradient (CG) algorithm is used to solve for the inversion of the linear operator in Line-4 in the algorithm above, where the linear operator is too large to store explicitly as a matrix (such as FFT/IFFT of an image) and invert directly. Instead, we can solve for the linear inversion iteratively as in CG. The SURE loss is applied for Line-6 above. This is a differentiable loss function that can be used to train/giude an operator (e.g. neural network), where the pseudo ground truth is available but the reference ground truth is not. For example, in the MRI reconstruction, the pseudo ground truth is the zero-filled reconstruction and the reference ground truth is the fully sampled reconstruction. The reference ground truth is not available due to the lack of fully sampled. **Reference** [1] Stein, C.M.: Estimation of the mean of a multivariate normal distribution. Annals of Statistics 1981 [[paper link](https://projecteuclid.org/journals/annals-of-statistics/volume-9/issue-6/Estimation-of-the-Mean-of-a-Multivariate-Normal-Distribution/10.1214/aos/1176345632.full)] [2] B. Ozturkler et al. SMRD: SURE-based Robust MRI Reconstruction with Diffusion Models. MICCAI 2023 [[paper link](https://arxiv.org/pdf/2310.01799.pdf)] ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [x] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [x] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [x] In-line docstrings updated. - [x] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: chaoliu <chaoliu@nvidia.com> Signed-off-by: cxlcl <chaoliucxl@gmail.com> Signed-off-by: chaoliu <chaoliucxl@gmail.com> Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
for more information, see https://pre-commit.ci Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
Signed-off-by: monai-bot <monai.miccai2019@gmail.com> Signed-off-by: monai-bot <monai.miccai2019@gmail.com> Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
1c10112 to
ae704f3
Compare
Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
…ject-MONAI#7569) Fixes Project-MONAI#7451 ### Description Reduces the length of error messages and error messages being propagated twice. This helps debug better when long `ConfigComponent`s are being instantiated. Refer to issue Project-MONAI#7451 for more details ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [x] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. Signed-off-by: Suraj Pai <b.pai@maastrichtuniversity.nl> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Fixes Project-MONAI#2872 ### Description Implementation of mixup, cutmix and cutout as described in the original papers. Current implementation support both, the dictionary-based batches and tuples of tensors. ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [x] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [x] In-line docstrings updated. - [x] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: Juan Pablo de la Cruz Gutiérrez <juampatronics@gmail.com> Signed-off-by: monai-bot <monai.miccai2019@gmail.com> Signed-off-by: elitap <elias.tappeiner@gmx.at> Signed-off-by: Felix Schnabel <f.schnabel@tum.de> Signed-off-by: YanxuanLiu <yanxuanl@nvidia.com> Signed-off-by: ytl0623 <david89062388@gmail.com> Signed-off-by: Dženan Zukić <dzenan.zukic@kitware.com> Signed-off-by: KumoLiu <yunl@nvidia.com> Signed-off-by: YunLiu <55491388+KumoLiu@users.noreply.github.com> Signed-off-by: Ishan Dutta <ishandutta0098@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: kaibo <ktang@unc.edu> Signed-off-by: heyufan1995 <heyufan1995@gmail.com> Signed-off-by: binliu <binliu@nvidia.com> Signed-off-by: axel.vlaminck <axel.vlaminck@gmail.com> Signed-off-by: Ibrahim Hadzic <ibrahimhadzic45@gmail.com> Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com> Signed-off-by: Timothy Baker <bakertim@umich.edu> Signed-off-by: Mathijs de Boer <m.deboer-41@umcutrecht.nl> Signed-off-by: Fabian Klopfer <fabian.klopfer@ieee.org> Signed-off-by: Lucas Robinet <robinet.lucas@iuct-oncopole.fr> Signed-off-by: Lucas Robinet <67736918+Lucas-rbnt@users.noreply.github.com> Signed-off-by: chaoliu <chaoliu@nvidia.com> Signed-off-by: cxlcl <chaoliucxl@gmail.com> Signed-off-by: chaoliu <chaoliucxl@gmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: monai-bot <64792179+monai-bot@users.noreply.github.com> Co-authored-by: elitap <elitap@users.noreply.github.com> Co-authored-by: Felix Schnabel <f.schnabel@tum.de> Co-authored-by: YanxuanLiu <104543031+YanxuanLiu@users.noreply.github.com> Co-authored-by: ytl0623 <david89062388@gmail.com> Co-authored-by: Dženan Zukić <dzenan.zukic@kitware.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com> Co-authored-by: Ishan Dutta <ishandutta0098@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Kaibo Tang <ktang@unc.edu> Co-authored-by: Yufan He <59374597+heyufan1995@users.noreply.github.com> Co-authored-by: binliunls <107988372+binliunls@users.noreply.github.com> Co-authored-by: Ben Murray <ben.murray@gmail.com> Co-authored-by: axel.vlaminck <axel.vlaminck@gmail.com> Co-authored-by: Mingxin Zheng <18563433+mingxin-zheng@users.noreply.github.com> Co-authored-by: Ibrahim Hadzic <ibrahimhadzic45@gmail.com> Co-authored-by: Dr. Behrooz Hashemian <3968947+drbeh@users.noreply.github.com> Co-authored-by: Timothy J. Baker <62781117+tim-the-baker@users.noreply.github.com> Co-authored-by: Mathijs de Boer <8137653+MathijsdeBoer@users.noreply.github.com> Co-authored-by: Mathijs de Boer <m.deboer-41@umcutrecht.nl> Co-authored-by: Fabian Klopfer <fabian.klopfer@ieee.org> Co-authored-by: Yiheng Wang <68361391+yiheng-wang-nv@users.noreply.github.com> Co-authored-by: Lucas Robinet <67736918+Lucas-rbnt@users.noreply.github.com> Co-authored-by: Lucas Robinet <robinet.lucas@iuct-oncopole.fr> Co-authored-by: cxlcl <chaoliucxl@gmail.com>
a65ef4a to
3264079
Compare
Signed-off-by: Dr. Behrooz Hashemian <3968947+drbeh@users.noreply.github.com>
|
@NikolasSchmitz, I resolved the conflicts and updated the PR. Let's focus on the functionality and make this PR ready for reviewing. We can take care of any other issue once the PR is ready and please feel free to reach out to me or the working group if you still have any questions. Thanks for taking on this feature. |
|
Thank you @drbeh ! I now marked it as ready for review. |
|
@drbeh would you be able to review this? I think you're best qualified here. I made some minor comments about print and code duplication but otherwise it seems fine to me without having tested it. Thanks! |
JHancox
left a comment
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.
Thanks for the effort @NikolasSchmitz. A welcome addition! I agree with @ericspod about the refactoring to remove duplication - that would be great.
|
Thank you for the feedback, @JHancox . Unfortunately, I couldn't join the meeting because I was on my way home from ICLR. |
… reduce redundancy; for get_mpp of TiffFileWSIReader: added check to prevent division by zero error.
|
Hi @NikolasSchmitz thanks for getting back to this. There's a few tests which still aren't passing, a few say just that a timeout occurred so it might be internal but the code formatting test did find something. As for DCO, clicking on that brings up advice on how to resolve, specifically doing an empty remedial commit. If that doesn't work we will set the DCO to pass just before merging. Please have a look at the tests and see if we can get that through. |
|
We also are putting more test data on Huggingface now so you can propose to upload your image to here https://huggingface.co/datasets/MONAI/testing_data if you like. We're planning to adjust our tests to pull from this dataset. |
|
Your new tiff file is added to the hf dataset now, so you need to change @skipUnless(has_cucim or has_osl or has_tiff, "Requires cucim, openslide, or tifffile!")
def setUpModule():
download_url_or_skip_test(
testing_data_config("images", WSI_GENERIC_TIFF_KEY, "url"),
WSI_GENERIC_TIFF_PATH,
hash_type=testing_data_config("images", WSI_GENERIC_TIFF_KEY, "hash_type"),
hash_val=testing_data_config("images", WSI_GENERIC_TIFF_KEY, "hash_val"),
)
download_url_or_skip_test(
testing_data_config("images", WSI_GENERIC_TIFF_CORRECT_MPP_KEY, "url"),
WSI_GENERIC_TIFF_CORRECT_MPP_PATH,
hash_type=testing_data_config("images", WSI_GENERIC_TIFF_CORRECT_MPP_KEY, "hash_type"),
hash_val=testing_data_config("images", WSI_GENERIC_TIFF_CORRECT_MPP_KEY, "hash_val"),
)
download_url_or_skip_test(
testing_data_config("images", WSI_APERIO_SVS_KEY, "url"),
WSI_APERIO_SVS_PATH,
hash_type=testing_data_config("images", WSI_APERIO_SVS_KEY, "hash_type"),
hash_val=testing_data_config("images", WSI_APERIO_SVS_KEY, "hash_val"),
)Add the following to "wsi_generic_tiff_corrected": {
"url": "https://huggingface.co/datasets/MONAI/testing_data/resolve/main/CMU-1_correct_mpp.tiff",
"hash_type": "sha256",
"hash_val": "65306e3f8f7f5282d19d942dadc525cd06a80d5fd8268053939751365226c65f"
},As you can see, our process for downloading data needs to be revamped to improved this process! Thanks. |
…en.de> I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: d0a4881 I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 7545148 I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 5c7822f I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 234f23f I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 9d817e7 I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 8270658 I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 23e4a74 I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 787d30f I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 0d9f1dd I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 45182fa I, Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>, hereby add my Signed-off-by to this commit: 832c14e Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
|
@NikolasSchmitz thanks for your contribution. Please let me know if you need help fixing the DCO issues. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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.
Actionable comments posted: 3
♻️ Duplicate comments (1)
monai/data/wsi_reader.py (1)
663-681: Validate mpp input early.Reject non-tuples, wrong length, or non-positive values to avoid undefined behavior downstream.
def get_wsi_at_mpp(self, wsi, mpp: tuple, atol: float = 0.00, rtol: float = 0.05) -> np.ndarray: @@ - return self.reader.get_wsi_at_mpp(wsi, mpp, atol, rtol) + # basic validation + if not isinstance(mpp, tuple) or len(mpp) != 2: + raise ValueError("mpp must be a tuple (x, y).") + if any((not isinstance(v, (int, float))) or v <= 0 for v in mpp): + raise ValueError("mpp values must be positive numbers.") + return self.reader.get_wsi_at_mpp(wsi, mpp, atol, rtol)
🧹 Nitpick comments (8)
tests/utils/enums/test_wsireader.py (2)
479-505: Strengthen assertions for cross-backend robustness.Exact H×W can vary a few pixels due to rounding/interp differences. Also assert:
- dtype and channel count per backend,
- monotonicity of area vs requested MPP,
- that returned MPP (via metadata or helper) is within tolerance.
Consider computing expected size from level-0 dims and requested mpp rather than hard-coding tuples to reduce brittleness.
467-478: Remove commented-out test or re-enable behind a marker.Dead/commented code adds noise. Delete or gate with a feature flag/xfail.
monai/data/wsi_reader.py (5)
843-846: Don’t pass hard-coded tolerances to _find_closest_level; avoid unintended behavior.The 0,5 literals silently decouple user-provided tolerances. Either:
- use the provided (atol, rtol), or
- bypass tolerance checking entirely when only the index of the closest level is needed.
Apply one of the following:
Option A (simple):
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)Option B (preferred: no early raising):
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + # Pick closest level without tolerance checks; actual tolerances are evaluated below. + closest_lvl = min(range(len(mpp_list)), + key=lambda i: abs(mpp_list[i][0]-mpp[0]) + abs(mpp_list[i][1]-mpp[1]))Also applies to: 1108-1111, 1345-1348
949-975: Normalize dimension handling to reduce fragility.closest_lvl_dim is backend-native (often (width, height)); _compute_mpp_target_res expects (height, width), leading to implicit swaps. Unify by passing self.get_size(wsi, closest_lvl) to _compute_mpp_target_res, and use explicit width/height when calling backend resizers.
Example for CuCIM (apply similarly to OpenSlide/TiffFile as appropriate):
- closest_lvl_dim = wsi.resolutions["level_dimensions"][closest_lvl] - target_res_x, target_res_y = self._compute_mpp_target_res(closest_lvl, closest_lvl_dim, mpp_list, user_mpp) - wsi_arr = cp.array(wsi.read_region((0, 0), level=closest_lvl, size=closest_lvl_dim, num_workers=self.num_workers)) + # Consistent dims: (h, w) + h, w = self.get_size(wsi, closest_lvl) + target_w, target_h = self._compute_mpp_target_res(closest_lvl, (h, w), mpp_list, user_mpp) + # cuCIM read_region expects (w, h) + wsi_arr = cp.array(wsi.read_region((0, 0), level=closest_lvl, size=(w, h), num_workers=self.num_workers)) - closest_lvl_wsi = cucim_resize( - wsi_arr, - (target_res_x, target_res_y), + # cucim.resize expects (rows, cols) == (h, w) + closest_lvl_wsi = cucim_resize( + wsi_arr, + (target_h, target_w), order=1, preserve_range=True, anti_aliasing=False).astype(cp.uint8)For OpenSlide’s PIL.resize, pass (target_w, target_h).
Also applies to: 1200-1221, 1442-1462
1314-1324: Normalize ResolutionUnit and tighten error handling.Lowercase the unit to match ConvertUnits expectations; use shorter messages (TRY003).
- convert_to_micron = ConvertUnits(unit, "micrometer") + convert_to_micron = ConvertUnits(str(unit).lower(), "micrometer") @@ - if xres[0] and yres[0]: + if xres[0] and yres[0]: return convert_to_micron(yres[1] / yres[0]), convert_to_micron(xres[1] / xres[0]) else: - raise ValueError("The `XResolution` and/or `YResolution` property of the image is zero, " - "which is needed to obtain `mpp` for this file. Please use `level` instead.") + raise ValueError("Invalid TIFF X/YResolution (zero). Cannot compute mpp; use `level` instead.")
960-974: Consider anti-aliasing when downscaling.Downscaling without anti_aliasing can introduce artifacts. Enable it when target size is smaller than source.
- closest_lvl_wsi = cucim_resize( + downscale = (target_h < h) and (target_w < w) + closest_lvl_wsi = cucim_resize( wsi_arr, (target_h, target_w), order=1, preserve_range=True, - anti_aliasing=False).astype(cp.uint8) + anti_aliasing=downscale).astype(cp.uint8)
1099-1105: Docstring: clarify tolerance interplay and behavior on miss.Explicitly state: closest level is selected ignoring tolerances; tolerances only decide “return as-is vs resize”. Document behavior when requested MPP is finer than level 0 (error vs upsample).
Also applies to: 1336-1342, 833-839
tests/testing_data/data_config.json (1)
8-12: Consider pinning to a specific commit for reproducibility.Hash verified ✓ and artifact accessible. However,
resolve/mainremains mutable; prefer pinning to a commit hash (e.g.,resolve/abc1234) for CI stability. The SHA256 guard mitigates risk but doesn't prevent future mutations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
monai/data/wsi_reader.py(9 hunks)monai/transforms/regularization/array.py(0 hunks)tests/testing_data/data_config.json(1 hunks)tests/utils/enums/test_wsireader.py(4 hunks)
💤 Files with no reviewable changes (1)
- monai/transforms/regularization/array.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/data/wsi_reader.pytests/utils/enums/test_wsireader.py
🪛 Ruff (0.14.1)
monai/data/wsi_reader.py
1322-1323: Avoid specifying long messages outside the exception class
(TRY003)
1324-1324: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (ubuntu-latest)
| TEST_CASE_SVS_MPP_1 = [ | ||
| WSI_APERIO_SVS_PATH, | ||
| {"mpp": (4.0, 4.0), "atol": 0.0, "rtol": 0.1}, | ||
| {"openslide": (4106, 5739, 4), "cucim": (4106, 5739, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_SVS_MPP_2 = [ | ||
| WSI_APERIO_SVS_PATH, | ||
| {"mpp": (8.0, 8.0)}, | ||
| {"openslide": (2057, 2875, 4), "cucim": (2057, 2875, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_SVS_MPP_3 = [ | ||
| WSI_APERIO_SVS_PATH, | ||
| {"mpp": (3.0, 3.0)}, | ||
| {"openslide": (5475, 7652, 4), "cucim": (5475, 7652, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_SVS_MPP_4 = [ | ||
| WSI_APERIO_SVS_PATH, | ||
| {"mpp": (1.5, 1.5)}, | ||
| {"openslide": (10949, 15303, 4), "cucim": (10949, 15303, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_TIFF_MPP_1 = [ | ||
| WSI_GENERIC_TIFF_CORRECT_MPP_PATH, | ||
| {"mpp": (4.0, 4.0), "atol": 0.0, "rtol": 0.1}, | ||
| {"openslide": (4114, 5750, 4), "cucim": (4114, 5750, 3), "tifffile": (4106, 5739, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_TIFF_MPP_2 = [ | ||
| WSI_GENERIC_TIFF_CORRECT_MPP_PATH, | ||
| {"mpp": (8.0, 8.0)}, | ||
| {"openslide": (2057, 2875, 4), "cucim": (2057, 2875, 3), "tifffile": (2053, 2869, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_TIFF_MPP_3 = [ | ||
| WSI_GENERIC_TIFF_CORRECT_MPP_PATH, | ||
| {"mpp": (3.0, 3.0)}, | ||
| {"openslide": (5475, 7652, 4), "cucim": (5475, 7652, 3), "tifffile": (5475, 7651, 3)}, | ||
| ] | ||
|
|
||
| TEST_CASE_TIFF_MPP_4 = [ | ||
| WSI_GENERIC_TIFF_CORRECT_MPP_PATH, | ||
| {"mpp": (1.5, 1.5)}, | ||
| {"openslide": (10949, 15303, 4), "cucim": (10949, 15303, 3), "tifffile": (10949, 15303, 3)}, | ||
| ] | ||
|
|
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.
🛠️ Refactor suggestion | 🟠 Major
Add a “too-fine MPP” test to catch boundary behavior.
Add a case where requested mpp is smaller than level-0 (i.e., finer than native). Current backend code decrements level and can underflow; a test will guard it.
I can add a parameterized TEST_CASE__MPP_TOO_FINE and expected exception once backend behavior is decided (error vs upsample).
Also applies to: 286-309
|
@NikolasSchmitz please let me know if I can help to finalize and merge this PR. Thanks |
|
Hey Bruce, I will finalize this PR in the next days and will let you know whether I need help. Thanks! |
|
/build |
…tz/MONAI into 4980-get-wsi-at-mpp
|
Hi @bhashemian, I’ve just pushed my current progress which includes two additions suggested by coderabbitai. Thanks! |
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.
Actionable comments posted: 9
♻️ Duplicate comments (5)
monai/data/wsi_reader.py (5)
843-843: Fix hardcoded tolerance values.Line uses hardcoded
(0, 5)instead of theatolandrtolparameters passed to the function. This bug was flagged in previous reviews.Apply this diff:
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)
1110-1110: Fix hardcoded tolerance values.Line uses hardcoded
(0, 5)instead of theatolandrtolparameters.Apply this diff:
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)
1124-1127: Guard against level underflow.Decrementing
closest_lvlwithout checking if it's 0 can cause underflow to -1.Apply this diff:
else: # If both checks fail, increase resolution (i.e., decrement level) and then downsample it. + if closest_lvl == 0: + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + else: - closest_lvl = closest_lvl - 1 - closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + closest_lvl = closest_lvl - 1 + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp)
1347-1347: Fix hardcoded tolerance values.Line uses hardcoded
(0, 5)instead of theatolandrtolparameters.Apply this diff:
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)
1359-1362: Guard against level underflow.Decrementing
closest_lvlwithout checking if it's 0 can cause underflow to -1.Apply this diff:
else: # If both checks fail, increase resolution (i.e., decrement level) and then downsample it. + if closest_lvl == 0: + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + else: - closest_lvl = closest_lvl - 1 - closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + closest_lvl = closest_lvl - 1 + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/data/wsi_reader.py(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/data/wsi_reader.py
🪛 Ruff (0.14.4)
monai/data/wsi_reader.py
1324-1325: Avoid specifying long messages outside the exception class
(TRY003)
1326-1326: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (1)
monai/data/wsi_reader.py (1)
1317-1326: Validation logic for zero resolution is correct.The check prevents division-by-zero when X/YResolution numerators are zero. Static analysis flagged the long exception message (TRY003), but this is acceptable for clarity.
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.
Actionable comments posted: 1
♻️ Duplicate comments (5)
monai/data/wsi_reader.py (5)
843-843: Fix hardcoded tolerance values.Uses hardcoded
(0, 5)instead of the function'satolandrtolparameters. This bug was flagged in past reviews but remains unfixed.Apply this diff:
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)
1110-1110: Fix hardcoded tolerance values.Uses hardcoded
(0, 5)instead of the function'satolandrtolparameters. Same bug as in CuCIMWSIReader.Apply this diff:
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)
1359-1362: Guard against level underflow.If
closest_lvlis 0, line 1361 decrements it to -1, causing negative indexing.Apply this diff:
else: # If both checks fail, increase resolution (i.e., decrement level) and then downsample it. - closest_lvl = closest_lvl - 1 - closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + if closest_lvl == 0: + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + else: + closest_lvl = closest_lvl - 1 + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp)
1347-1347: Fix hardcoded tolerance values.Uses hardcoded
(0, 5)instead of the function'satolandrtolparameters. Same bug as in other backends.Apply this diff:
- closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) + closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, atol, rtol)
1124-1127: Guard against level underflow.If
closest_lvlis 0, line 1126 decrements it to -1, causing negative indexing into the level list.Apply this diff:
else: # If both checks fail, increase resolution (i.e., decrement level) and then downsample it. - closest_lvl = closest_lvl - 1 - closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + if closest_lvl == 0: + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) + else: + closest_lvl = closest_lvl - 1 + closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp)
🧹 Nitpick comments (3)
monai/data/wsi_reader.py (3)
434-455: Document return type in docstring.The method returns
tuple[int, int](target width, height) but this isn't specified in the docstring.Add to docstring:
Args: closest_lvl: Whole slide image level closest to user-provided MPP resolution. closest_lvl_dim: Dimensions (height, width) of the image at the closest level. mpp_list: List of MPP values for all levels of the whole slide image. mpp: The MPP resolution at which the whole slide image representation should be extracted. + + Returns: + tuple[int, int]: Target dimensions (width, height) for the resized image.
457-489: Document return type in docstring.The method returns
tuple[bool, bool]but this isn't specified in the docstring.Add to docstring:
Args: closest_lvl: Whole slide image level closest to user-provided MPP resolution. mpp_list: List of MPP values for all levels of the whole slide image. mpp: The MPP resolution at which the whole slide image representation should be extracted. atol: Absolute tolerance for MPP comparison. rtol: Relative tolerance for MPP comparison. + + Returns: + tuple[bool, bool]: (is_within_tolerance, closest_level_is_bigger)
951-977: Minor formatting: add space before method call.Line 975 missing space before
.astype.Apply this diff:
(target_res_x, target_res_y), order=1, preserve_range=True, - anti_aliasing=False).astype(cp.uint8) + anti_aliasing=False + ).astype(cp.uint8)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/data/wsi_reader.py(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/data/wsi_reader.py
🪛 Ruff (0.14.4)
monai/data/wsi_reader.py
1324-1325: Avoid specifying long messages outside the exception class
(TRY003)
1326-1326: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
🔇 Additional comments (3)
monai/data/wsi_reader.py (3)
1202-1222: LGTM.Resizing logic is correct and uses appropriate PIL interpolation.
1317-1326: LGTM - division-by-zero guard added.The validation correctly prevents division by zero when TIFF resolution tags contain zero numerators. The static analysis hint (TRY003) about exception message length is pedantic and can be ignored here.
1444-1464: LGTM.Resizing logic is correct and consistent with OpenSlide implementation.
Signed-off-by: Nikolas Schmitz <nikolas.schmitz@rwth-aachen.de>
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/data/wsi_reader.py (1)
460-496: Fix return type annotation.
_compute_mpp_tolerancesactually returns a pair of booleans; the annotation-> boolis misleading and confuses type checkers. Update it totuple[bool, bool]to match the real contract.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/data/wsi_reader.py(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/data/wsi_reader.py
🪛 Ruff (0.14.4)
monai/data/wsi_reader.py
1350-1351: Avoid specifying long messages outside the exception class
(TRY003)
1352-1352: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
| def get_wsi_at_mpp(self, wsi, mpp: tuple, atol: float = 0.00, rtol: float = 0.05) -> Any: | ||
| """ | ||
| Returns the representation of the whole slide image at a given micro-per-pixel (mpp) resolution. | ||
| The optional tolerance parameters are considered at the level whose mpp value is closest to the one provided by the user. | ||
| If the user-provided mpp is larger than the mpp of the closest level, | ||
| the image is downscaled to a resolution that matches the user-provided mpp. | ||
| Otherwise, if the closest level's resolution is not sufficient to meet the user's requested resolution, | ||
| the next lower level (which has a higher resolution) is chosen. | ||
| The image from this level is then down-scaled to achieve a resolution at the user-provided mpp value. | ||
| Args: | ||
| wsi: whole slide image object from WSIReader | ||
| mpp: the resolution in micron per pixel at which the representation of the whole slide image should be extracted. | ||
| atol: the acceptable absolute tolerance for resolution in micro per pixel. | ||
| rtol: the acceptable relative tolerance for resolution in micro per pixel. | ||
| Returns: | ||
| Cupy array containing the whole slide image at the requested MPP resolution. | ||
| """ | ||
| cp, _ = optional_import("cupy") | ||
|
|
||
| mpp_list = [self.get_mpp(wsi, lvl) for lvl in range(wsi.resolutions["level_count"])] | ||
| closest_lvl = self._find_closest_level("mpp", mpp, mpp_list, 0, 5) | ||
|
|
||
| within_tolerance, closest_level_is_bigger = self._compute_mpp_tolerances(closest_lvl, mpp_list, mpp, atol, rtol) | ||
|
|
||
| if within_tolerance: | ||
| # If the image at the desired mpp resolution is within tolerances, return the image at closest_level. | ||
| closest_lvl_wsi = wsi.read_region( | ||
| (0, 0), level=closest_lvl, size=wsi.resolutions["level_dimensions"][closest_lvl], num_workers=self.num_workers | ||
| ) | ||
|
|
||
| elif closest_level_is_bigger: | ||
| # Otherwise, select the level closest to the desired mpp with a higher resolution and downsample it. | ||
| closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) | ||
|
|
||
| else: | ||
| # If both checks fail, increase resolution (i.e., decrement level) and then downsample it. | ||
| if closest_lvl == 0: | ||
| closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) | ||
| else: | ||
| closest_lvl = closest_lvl - 1 | ||
| closest_lvl_wsi = self._resize_to_mpp_res(wsi, closest_lvl, mpp_list, mpp) | ||
|
|
||
| wsi_arr = cp.asnumpy(closest_lvl_wsi) | ||
| return wsi_arr |
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.
Handle scalar mpp inputs.
Passing a single float (supported elsewhere in this API surface) now breaks because _find_closest_level calls len(value) on the scalar and raises TypeError. Normalize mpp with ensure_tuple_rep(mpp, 2) before using it here, and mirror the guard in the OpenSlide and TiffFile implementations so direct backend use stays compatible with existing call patterns.
Apply this diff (and replicate in the other backends):
cp, _ = optional_import("cupy")
+ mpp = ensure_tuple_rep(mpp, 2)
+
mpp_list = [self.get_mpp(wsi, lvl) for lvl in range(wsi.resolutions["level_count"])]🤖 Prompt for AI Agents
In monai/data/wsi_reader.py around lines 833 to 879, the function get_wsi_at_mpp
fails when mpp is passed as a scalar because _find_closest_level expects a tuple
and calls len(value); normalize mpp at the start of the function using
ensure_tuple_rep(mpp, 2) (same guard used in OpenSlide/TiffFile backends) so
subsequent calls (mpp_list computation and _find_closest_level) always receive a
2-tuple; apply the same normalization to the other backend implementations to
keep direct backend usage compatible with existing call patterns.
Fixes: #4980
Description
In this pull request, the feature to retrieve a whole slide image at a given mpp (microns per pixel) resolution was implemented for every WSIReader class in the function
get_wsi_at_mpp.While the implementations in the
OpenslideWSIReaderandCuCIMWSIReaderclasses were tested thoroughly, I could not find a suitable TIFF file for testing with theTiffFileWSIReaderclass.For resizing, I have used
PIL.Image.resizefor Openslide and TiffFile, andcucim.sklearn.transform.resizefor CuCIM. Originally, I usedcv2.resize, but since the package isn't listed in requirements-dev.txt, I explored alternative solutions."Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.Please let me know what you think and how I can improve this feature.
Best
Niko
(Last PR was closed, because I changed the branch name to include the ticket id)