Skip to content
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

🩹 Support for np.ndarray and WSIReader in PatchPredictor #576

Closed
wants to merge 25 commits into from
Closed

🩹 Support for np.ndarray and WSIReader in PatchPredictor #576

wants to merge 25 commits into from

Conversation

blaginin
Copy link
Collaborator

@blaginin blaginin commented Mar 27, 2023

This PR fixes #532 and improves PatchPredictor readability and intuitiveness.

What has changed

  • PatchPredictor can now work with file paths and NumPy arrays, as declared in the docs. Now it's possible to do this:
>>> predictor = PatchPredictor(pretrained_model="resnet18-kather100k", batch_size=32)
>>> normalizer = get_normalizer("reinhard")
>>> normalizer.fit(wsi.slide_thumbnail())

>>> predictor.predict(
...    input_imgs=[normalizer.transform(tile.slide_thumbnail(1, "baseline"))],  # 👀
...    mode="tile",
...    merge_predictions=True,
...    patch_input_shape=[224, 224],
...    stride_shape=[224, 224],
...    resolution=1,
...    units="baseline",
...    return_probabilities=True,
...    on_gpu=False
... )
  • Transformation to baseline ioconfig is now explicit. predict function used to disregard user-defined arguments, significantly impacting how the data is processed. Although it sent a warning, this warning can easily drown among the messages from pytorch/torchvision/wsireader/... especially when processing many files at once. Users' direct instructions should be changed only if we are 100% sure about the results. Since it's not the case here, instead of a warning user will receive an exception:
>>> W, H = 100, 200
>>> predictor = PatchPredictor(pretrained_model="resnet18-kather100k", batch_size=32)
>>> data = (np.arange(W * H * 3).reshape((W, H, 3)) % 255).astype(np.uint8)
>>> reader = WSIReader.open(data)  # we don't know MPP of the data

>>> predictor.predict(
...    input_imgs=[reader],
...    mode="wsi",
...    merge_predictions=True,
...    patch_input_shape=[224, 224],
...    stride_shape=[224, 224],
...    resolution=1,
...    units="mpp",  # 👀
...    return_probabilities=True,
...    on_gpu=False
... )

ValueError: MPP is None. Cannot determine scale in terms of MPP.

I also refactored the code:

  • WSIPatchDataset and VirtualWSIReader both operated on image-level data via OpenCV. Duplicated logic is removed, and now WSIPatchDataset works with images through WSIReader abstraction.
  • Removed a magic number.

Depends on #583

@blaginin blaginin marked this pull request as draft March 27, 2023 10:10
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #576 (11674c4) into develop (1c42e39) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #576   +/-   ##
========================================
  Coverage    99.77%   99.77%           
========================================
  Files           63       63           
  Lines         6782     6794   +12     
  Branches      1352     1362   +10     
========================================
+ Hits          6767     6779   +12     
  Misses           7        7           
  Partials         8        8           
Impacted Files Coverage Δ
tiatoolbox/models/dataset/classification.py 100.00% <100.00%> (ø)
tiatoolbox/models/dataset/dataset_abc.py 97.14% <100.00%> (+0.04%) ⬆️
tiatoolbox/models/engine/patch_predictor.py 100.00% <100.00%> (ø)
tiatoolbox/tools/patchextraction.py 100.00% <100.00%> (ø)
tiatoolbox/wsicore/wsireader.py 99.43% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shaneahmed shaneahmed added documentation Improvements or additions to documentation bug Something isn't working labels Mar 28, 2023
@shaneahmed
Copy link
Member

Hi @blaginin, Please can you break this down into smaller PRs? e.g., introducing blank sample can be a separate PR. Fixing documentation and support for np.ndarry can be separated as well. It will help with review.

@shaneahmed shaneahmed changed the title 🩹 Support for np.ndarray and WSIReader in PatchPredictor 🩹 Support for np.ndarray and WSIReader in PatchPredictor Mar 31, 2023
@blaginin blaginin marked this pull request as ready for review April 12, 2023 19:04
@blaginin
Copy link
Collaborator Author

Hi @blaginin, Please can you break this down into smaller PRs? e.g., introducing blank sample can be a separate PR. Fixing documentation and support for np.ndarry can be separated as well. It will help with review.

@shaneahmed excluded extra stuff, as we discussed

Copy link
Member

@shaneahmed shaneahmed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some initial comments.

requirements.txt Outdated
@@ -14,7 +14,7 @@ opencv-python>=4.6.0
openslide-python>=1.2.0
pandas>=2.0.0
pillow>=9.3.0
pydicom>=2.3.1 # Used by wsidicom
pydicom>=2.3.1 # Used by wsidef test_store_reader_no_types(tmp_path, remote_sample):dicom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wsidicom dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now requirements.txt is identical to develop

def predict(
self,
imgs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not update these as these need to be consistent with SemanticSegmentor and other engines. This will be fixed in redesigned engines #578

tiatoolbox/wsicore/wsireader.py Show resolved Hide resolved
@@ -486,7 +504,7 @@ def find_read_rect_params(
location: IntPair,
size: IntPair,
resolution: Resolution,
units: str,
units: Units,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are minor edits which can be done in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #598

@@ -951,7 +969,7 @@ def _read_rect_at_resolution(
location: NumPair,
size: NumPair,
resolution: Resolution = 0,
units: str = "level",
units: Units = "level",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. These kind of edits can be suggested in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #598

blaginin and others added 4 commits May 2, 2023 22:59
# Conflicts:
#	tiatoolbox/models/dataset/classification.py
#	tiatoolbox/models/engine/patch_predictor.py
@shaneahmed
Copy link
Member

#635 has restructured EngineABC to support numpy arrays, WSI and tile support will be added around this later. Is this what you intended with this PR? You can decouple changes to WSIPatchDataset and push those changes in a separate PR.

@blaginin blaginin closed this by deleting the head repository Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation inconistency
2 participants