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.

Sign up for GitHub

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

Jump to bottom

Bug fix and improvement in WSI #4216

Merged
merged 27 commits into from
May 6, 2022
Merged

Bug fix and improvement in WSI #4216

merged 27 commits into from
May 6, 2022

Conversation

bhashemian
Copy link
Member

@bhashemian bhashemian commented May 2, 2022 •
edited
Loading

Description

Fixes a bug and makes some updates in WSIReader and PatchWSIDataset

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).

bhashemian and others added 19 commits April 27, 2022 18:12
@bhashemian
Make all transforms optional
f6716e2 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Merge branch 'dev' into fix-smartcache
6506edd
@bhashemian
Merge branch 'dev' into fix-smartcache
ee59746
@bhashemian
Merge branch 'dev' of github.com:Project-MONAI/MONAI into fix-smartcache
aa08be9
@bhashemian
Update wsireader tests
f21fe5a 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Remove optional from PersistentDataset and its derivatives
1610bbc 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Add unittests for cache without transform
3d9516d 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Merge branch 'fix-smartcache' of github.com:behxyz/MONAI into fix-sma…
b1656f8 
…rtcache
@bhashemian
Add default replace_rate
a9f24c5 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Add default value
0d6450b 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Set default replace_rate to 0.1
20c4882 
Signed-off-by: Behrooz <[email protected]>
@wyli
Merge branch 'dev' into fix-smartcache
7096ad8
@bhashemian
Update metadata to include path
f586386 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Adds SmartCachePatchWSIDataset
9dc6ca4 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Add unittests for SmartCachePatchWSIDataset
6ad1b30 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Update references
c548750 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Merge branch 'fix-smartcache' of github.com:behxyz/MONAI into fix-sma…
e52d846 
…rtcache
@bhashemian
Merge branch 'dev' of github.com:Project-MONAI/MONAI into fix-smartcache
bee7f87
@bhashemian
Update docs
c874f8e 
Signed-off-by: Behrooz <[email protected]>
@bhashemian bhashemian requested a review from Nic-Ma May 2, 2022 15:35
@bhashemian
Remove smart cache
21e08c4 
Signed-off-by: Behrooz <[email protected]>
@bhashemian bhashemian changed the title Implement SmartCache for PatchWSIDataset Bug fix and doc update WSI May 2, 2022
@bhashemian bhashemian changed the title Bug fix and doc update WSI Bug fix and improvement and doc update WSI May 2, 2022
@bhashemian
Remove unused imports
a9de037 
Signed-off-by: Behrooz <[email protected]>
@bhashemian
Copy link
Member Author

@Nic-Ma, I removed SmartCacheWSIDataset from this PR and kept only the minor improvement and bug fix. Can you please review it?

@bhashemian bhashemian enabled auto-merge (squash) May 2, 2022 16:58
@bhashemian bhashemian requested a review from wyli May 2, 2022 19:01
@bhashemian bhashemian changed the title Bug fix and improvement and doc update WSI Bug fix and improvement in WSI May 2, 2022
@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 3, 2022

Hi @drbeh ,

I think we don't have missing action items for moving the existing WSI components to core, could you please help also unify test_wsireader_new.py and test_wsireader.py in this PR?
Please feel free to correct me if I misunderstood anything.

Thanks in advance.

Nic-Ma
Nic-Ma reviewed May 3, 2022
View reviewed changes
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
@bhashemian
Copy link
Member Author

Hi @drbeh ,

I think we don't have missing action items for moving the existing WSI components to core, could you please help also unify test_wsireader_new.py and test_wsireader.py in this PR? Please feel free to correct me if I misunderstood anything.

Thanks in advance.

Hi @Nic-Ma, I don't think that we should combine them in a single file since they are two different components with similar names. What I am planing to do is following:

  1. First, update the pipelines and tutorials with the new components and make sure that they are working as expected.
  2. Next, deprecate the old components.
  3. Finally, rename test_xxx.py -> test_xxx_depricated.py and test_xxx_new.py -> test_xxx.py.

Please let me know if you think differently.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 3, 2022

Hi @drbeh ,

I feel we may not need to keep the deprecated APIs, delete the APIs and tests directly?
@wyli What do you think?

Thanks.

@bhashemian
Copy link
Member Author

Hi @drbeh ,

I feel we may not need to keep the deprecated APIs, delete the APIs and tests directly? @wyli What do you think?

Thanks.

Deleting a component directly is not a good practice. The way that is usually done is to deprecate the component first to notify users (via the deprecation message) and give them time to update their code. Then after a period of time delete them all.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 3, 2022

Hi @drbeh ,

I understand the deprecate process, we already have several deprecated modules, for example:
https://github.com/Project-MONAI/MONAI/blob/dev/monai/data/png_writer.py#L22
deprecate may cause the code hard to read, so sometimes we just make breaking changes directly, depends how hard users switch to the new APIs.
Anyway, it's not a blocker issue, if @wyli @ericspod you guys also like the deprecate way, I am OK with it.

Thanks in advance.

@bhashemian
Copy link
Member Author

Hi @Nic-Ma, I'll create an issue to continue the discussion about deprecating the pathology components or not. But do you have any other comment for this PR? thanks

@bhashemian bhashemian mentioned this pull request May 4, 2022
Closed
@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 4, 2022

Hi @drbeh ,

We are still in Labor's Day holiday today, I will try to review it again later.
You can ask @wyli @ericspod to review it first.

Thanks.

@bhashemian
Copy link
Member Author

Sure, @Nic-Ma, enjoy your holiday! :)

Nic-Ma
Nic-Ma approved these changes May 5, 2022
View reviewed changes
Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Thanks.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented May 5, 2022

/build

@wyli
Copy link
Contributor

wyli commented May 5, 2022

the PR description says it fixes a bug, was wondering if the relevant test cases should be updated to cover the change?

@wyli wyli disabled auto-merge May 5, 2022 04:20
@bhashemian
Copy link
Member Author

the PR description says it fixes a bug, was wondering if the relevant test cases should be updated to cover the change?

That's a great point @wyli! I'll add the test.

@wyli
Copy link
Contributor

wyli commented May 6, 2022

/build

@wyli wyli enabled auto-merge (squash) May 6, 2022 17:44
@wyli wyli merged commit 209db9e into Project-MONAI:dev May 6, 2022
@bhashemian bhashemian deleted the fix-smartcache branch May 6, 2022 19:46
Can-Zhao pushed a commit to Can-Zhao/MONAI that referenced this pull request May 10, 2022
@bhashemian @Can-Zhao
Bug fix and improvement in WSI (Project-MONAI#4216)
6a22ed6 
* Make all transforms optional

Signed-off-by: Behrooz <[email protected]>

* Update wsireader tests

Signed-off-by: Behrooz <[email protected]>

* Remove optional from PersistentDataset and its derivatives

Signed-off-by: Behrooz <[email protected]>

* Add unittests for cache without transform

Signed-off-by: Behrooz <[email protected]>

* Add default replace_rate

Signed-off-by: Behrooz <[email protected]>

* Add default value

Signed-off-by: Behrooz <[email protected]>

* Set default replace_rate to 0.1

Signed-off-by: Behrooz <[email protected]>

* Update metadata to include path

Signed-off-by: Behrooz <[email protected]>

* Adds SmartCachePatchWSIDataset

Signed-off-by: Behrooz <[email protected]>

* Add unittests for SmartCachePatchWSIDataset

Signed-off-by: Behrooz <[email protected]>

* Update references

Signed-off-by: Behrooz <[email protected]>

* Update docs

Signed-off-by: Behrooz <[email protected]>

* Remove smart cache

Signed-off-by: Behrooz <[email protected]>

* Remove unused imports

Signed-off-by: Behrooz <[email protected]>

* Add path metadata for OpenSlide

Signed-off-by: Behrooz <[email protected]>

* Update metadata to be unified across different backends

Signed-off-by: Behrooz <[email protected]>

* Update wsi metadata for multi wsi objects

Signed-off-by: Behrooz <[email protected]>

* Add unittests for wsi metadata

Signed-off-by: Behrooz <[email protected]>
wyli pushed a commit that referenced this pull request May 13, 2022
@kbressem
Implement NrrdReader (#4259)
0c243b9 
* add swin_unetr model (#4074)

* add swin_unetr model

Signed-off-by: kbressem <[email protected]>

* 4217 Update PyTorch docker to 22.04 (#4218)

* [DLMED] update to 22.04

Signed-off-by: Nic Ma <[email protected]>

* fixes unit test tests.test_lr_finder

Signed-off-by: Wenqi Li <[email protected]>

* test new_empty

Signed-off-by: Wenqi Li <[email protected]>

Co-authored-by: Wenqi Li <[email protected]>
Co-authored-by: Wenqi Li <[email protected]>
Signed-off-by: kbressem <[email protected]>

* Add InstanceNorm3dNVFuser support (#4194)

* implement the base class

Signed-off-by: Yiheng Wang <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add unittest

Signed-off-by: Yiheng Wang <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* autofix

Signed-off-by: Yiheng Wang <[email protected]>

* switch to call apex directly

Signed-off-by: Yiheng Wang <[email protected]>

* uncomment unittest

Signed-off-by: Yiheng Wang <[email protected]>

* add apex install link in docstring

Signed-off-by: Yiheng Wang <[email protected]>

* add channels_last_3d test case

Signed-off-by: Yiheng Wang <[email protected]>

* rewrite types

Signed-off-by: Yiheng Wang <[email protected]>

* change types

Signed-off-by: Yiheng Wang <[email protected]>

* add docstrings

Signed-off-by: Yiheng Wang <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Wenqi Li <[email protected]>
Signed-off-by: kbressem <[email protected]>

* Update dice.py (#4234)

* Update dice.py

reduce redundant operations in DiceFocalLoss, initially caused oom

Signed-off-by: Ryan Clanton <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Signed-off-by: Ryan Clanton <[email protected]>

* [MONAI] python code formatting

Signed-off-by: monai-bot <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: monai-bot <[email protected]>
Signed-off-by: kbressem <[email protected]>

* Bug fix and improvement in WSI (#4216)

* Make all transforms optional

Signed-off-by: Behrooz <[email protected]>

* Update wsireader tests

Signed-off-by: Behrooz <[email protected]>

* Remove optional from PersistentDataset and its derivatives

Signed-off-by: Behrooz <[email protected]>

* Add unittests for cache without transform

Signed-off-by: Behrooz <[email protected]>

* Add default replace_rate

Signed-off-by: Behrooz <[email protected]>

* Add default value

Signed-off-by: Behrooz <[email protected]>

* Set default replace_rate to 0.1

Signed-off-by: Behrooz <[email protected]>

* Update metadata to include path

Signed-off-by: Behrooz <[email protected]>

* Adds SmartCachePatchWSIDataset

Signed-off-by: Behrooz <[email protected]>

* Add unittests for SmartCachePatchWSIDataset

Signed-off-by: Behrooz <[email protected]>

* Update references

Signed-off-by: Behrooz <[email protected]>

* Update docs

Signed-off-by: Behrooz <[email protected]>

* Remove smart cache

Signed-off-by: Behrooz <[email protected]>

* Remove unused imports

Signed-off-by: Behrooz <[email protected]>

* Add path metadata for OpenSlide

Signed-off-by: Behrooz <[email protected]>

* Update metadata to be unified across different backends

Signed-off-by: Behrooz <[email protected]>

* Update wsi metadata for multi wsi objects

Signed-off-by: Behrooz <[email protected]>

* Add unittests for wsi metadata

Signed-off-by: Behrooz <[email protected]>
Signed-off-by: kbressem <[email protected]>

* Replace module (#4245)

* replace modules

Signed-off-by: Richard Brown <[email protected]>

* fix

Signed-off-by: Richard Brown <[email protected]>

* replace_module -> replace_modules

Signed-off-by: Richard Brown <[email protected]>

* fix

Signed-off-by: Richard Brown <[email protected]>
Signed-off-by: kbressem <[email protected]>

* Add GaussianSmooth as antialiasing filter in Resize (#4249)

Signed-off-by: Can Zhao <[email protected]>
Signed-off-by: kbressem <[email protected]>

* 4235 fix 2204 nvfuser issue (#4241)

* reproduce issue

Signed-off-by: Yiheng Wang <[email protected]>

* remove 22.01 02

Signed-off-by: Yiheng Wang <[email protected]>

* remove other workflows

Signed-off-by: Yiheng Wang <[email protected]>

* run on pull request

Signed-off-by: Yiheng Wang <[email protected]>

* remove sleep

Signed-off-by: Yiheng Wang <[email protected]>

* test single layer forward

Signed-off-by: Yiheng Wang <[email protected]>

* add has_nvfuser

Signed-off-by: Yiheng Wang <[email protected]>

* add check within factory

Signed-off-by: Yiheng Wang <[email protected]>

* revert to original cron.yml

Signed-off-by: Yiheng Wang <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix old pt issue

Signed-off-by: Yiheng Wang <[email protected]>

* change to return directly if no cuda

Signed-off-by: Yiheng Wang <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: kbressem <[email protected]>

* Update to Bundle Specifiation (#4250)

* Update to bundle specifiation

Signed-off-by: Eric Kerfoot <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Adding description in spec discussing the saved Torchscript object's file storage behaviour, and tweaking ckpt_export to add .json extension

Signed-off-by: Eric Kerfoot <[email protected]>

* Annotating optional bundle files

Signed-off-by: Eric Kerfoot <[email protected]>

* Adjusted ckpt_export test

Signed-off-by: Eric Kerfoot <[email protected]>

* Fix

Signed-off-by: Eric Kerfoot <[email protected]>

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: kbressem <[email protected]>

* Implement NrrdReader and NrrdImage classes

Signed-off-by: kbressem <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Signed-off-by: kbressem <[email protected]>

* run auto style fixes on image_reader.py

Signed-off-by: kbressem <[email protected]>

* add NrrdReader to monai/data/__init__.py

Signed-off-by: kbressem <[email protected]>

* Change the way spatial information is handled in NrrdReader

Signed-off-by: kbressem <[email protected]>

* add tests for NrrdReader

Signed-off-by: kbressem <[email protected]>

* Add NrrdReader to list of possible readers for LoadImage

Signed-off-by: kbressem <[email protected]>

* autofix formating

Signed-off-by: kbressem <[email protected]>

* autofix formating

Signed-off-by: kbressem <[email protected]>

* change NrrdImage class to namedtuple and make flake8 happy

Signed-off-by: kbressem <[email protected]>

* Add pynrrd to requirements

Signed-off-by: kbressem <[email protected]>

* correct typing for namedtumple
make flake8 happy

Signed-off-by: kbressem <[email protected]>

* Add pynrrd info to `get_optional_config_values`
Changed NrrdImage to dataclass

Signed-off-by: kbressem <[email protected]>

* exclude test_nrrd_reader.py from min tests

Signed-off-by: kbressem <[email protected]>

* add pynrrd to config files

Signed-off-by: kbressem <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Change the way space is handled in the header. Now, if space is not in header, it is assumed to be LPS and converted to RAS. If space is defined and not LPS, nothing is done to prevent wrong conversions.

Signed-off-by: kbressem <[email protected]>

* add `TestLoadSaveNrrd` where it is tested if a nrrd file, created by ITKWriter can be loaded again. 2D and 3D files with no channels are tested

Signed-off-by: kbressem <[email protected]>

* autofix format


Co-authored-by: kbressem <[email protected]>
@bhashemian bhashemian self-assigned this Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nic-Ma Nic-Ma Nic-Ma approved these changes

@wyli wyli Awaiting requested review from wyli

Assignees

@bhashemian bhashemian

Labels
None yet
Projects
AI in Pathology🔬
Status: 💯 Complete
Milestone
Blend pathology components into core ...
Development

Successfully merging this pull request may close these issues.
3 participants
@bhashemian @Nic-Ma @wyli