Skip to content

Update WSIReader for CuCIM#1876

Merged
bhashemian merged 9 commits intoProject-MONAI:masterfrom
bhashemian:update_wsireader_cucim
Mar 31, 2021
Merged

Update WSIReader for CuCIM#1876
bhashemian merged 9 commits intoProject-MONAI:masterfrom
bhashemian:update_wsireader_cucim

Conversation

@bhashemian
Copy link
Copy Markdown
Member

@bhashemian bhashemian commented Mar 27, 2021

Fixes #1875

Description

This PR update how the size is passed to CuCIM to enable extraction of whole images at any level. Also handle conversion of RGBA to RGB for CuCIM

Status

Ready

Types of changes

  • 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.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@bhashemian bhashemian requested review from Nic-Ma, rijobro and wyli March 27, 2021 22:27
@bhashemian bhashemian force-pushed the update_wsireader_cucim branch from 8e97993 to 30b6ea8 Compare March 27, 2021 22:29
@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Mar 29, 2021

Hi @wyli ,

Could you please help double confirm the new logic?

Thanks.

wyli
wyli reviewed Mar 29, 2021
View reviewed changes
@bhashemian bhashemian enabled auto-merge (squash) March 29, 2021 15:08
@bhashemian bhashemian mentioned this pull request Mar 29, 2021
Merged
7 tasks
@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 29, 2021 •
edited
Loading

@wyli is there anything else that I can address for merging this?

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 29, 2021

@wyli is there anything else that I can address for merging this?

perhaps could address the camelyon_data_download issue in this PR, since this one is relatively simple.
also please add some tests to cover the RGBA change.

@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 29, 2021

@wyli is there anything else that I can address for merging this?

perhaps could address the camelyon_data_download issue in this PR, since this one is relatively simple.
also please add some tests to cover the RGBA change.

OK!

@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 29, 2021

@wyli, I need to think a test case for RGBA, but I added what you asked for data download here #1892

@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 30, 2021

@wyli, I added the test case to cover RGBA.

wyli
wyli approved these changes Mar 30, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@wyli wyli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks! could you please add the "size": None case to the test_openslide_reader and then I think this PR is ready

@bhashemian
Copy link
Copy Markdown
Member Author

bhashemian commented Mar 30, 2021

thanks! could you please add the "size": None case to the test_openslide_reader and then I think this PR is ready

@wyli, I believe it is already there:

MONAI/tests/test_openslide_reader.py

Line 22 in 43f482c

TEST_CASE_0 = [FILE_PATH, (3, HEIGHT, WIDTH)]

@wyli
Copy link
Copy Markdown
Contributor

wyli commented Mar 30, 2021

thanks! could you please add the "size": None case to the test_openslide_reader and then I think this PR is ready

@wyli, I believe it is already there:

MONAI/tests/test_openslide_reader.py

Line 22 in 43f482c

TEST_CASE_0 = [FILE_PATH, (3, HEIGHT, WIDTH)]

ok I see, the pre-merge quick tests never run the openslide related ones as there's no installation. please make sure it works locally for the rest PRs

@bhashemian bhashemian merged commit 1612ec3 into Project-MONAI:master Mar 31, 2021
@bhashemian bhashemian deleted the update_wsireader_cucim branch April 12, 2021 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wyli wyli wyli approved these changes

@Nic-Ma Nic-Ma Awaiting requested review from Nic-Ma

@rijobro rijobro Awaiting requested review from rijobro

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bring CuCIM updated features to WSIReader

3 participants

@bhashemian @Nic-Ma @wyli