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

WSIReader read by power and mpp #6244

Merged
merged 41 commits into from
Apr 11, 2023
Merged

WSIReader read by power and mpp #6244

merged 41 commits into from
Apr 11, 2023

Conversation

drbeh
Copy link
Member

@drbeh drbeh commented Mar 27, 2023

Fixes #6289

Description

This PR enable WSIReader to be provided with objective power and resolution (micron per pixel) to decide which WSI level to load. It also set absolute and relative tolerances for power and mpp.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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 --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@drbeh drbeh marked this pull request as ready for review March 27, 2023 23:03
@drbeh drbeh requested review from wyli, KumoLiu and Nic-Ma March 27, 2023 23:04
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
tests/test_wsireader.py Outdated Show resolved Hide resolved
@drbeh drbeh requested a review from KumoLiu March 29, 2023 20:10
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
monai/data/wsi_reader.py Show resolved Hide resolved
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
@drbeh
Copy link
Member Author

drbeh commented Apr 4, 2023

@Nic-Ma @wyli please let me know if you have any other comments. Otherwise, this PR is ready to be merged. Thanks

@drbeh drbeh mentioned this pull request Apr 4, 2023
1 task
monai/data/wsi_reader.py Outdated Show resolved Hide resolved
@wyli
Copy link
Contributor

wyli commented Apr 4, 2023

this missing piece is an option for handling the resampling e.g. when the requested exact mpp is not available. could you please add this?

@wyli that's correct! We have discussed about it in the pathology working group few weeks ago and we decided not to include resampling in this PR and leave it as a feature request for the future since it requires some decision on how and when exactly to do resampling.

so in that case this PR wouldn't close #4980 as the description suggested?

@drbeh
Copy link
Member Author

drbeh commented Apr 4, 2023

this missing piece is an option for handling the resampling e.g. when the requested exact mpp is not available. could you please add this?

@wyli that's correct! We have discussed about it in the pathology working group few weeks ago and we decided not to include resampling in this PR and leave it as a feature request for the future since it requires some decision on how and when exactly to do resampling.

so in that case this PR wouldn't close #4980 as the description suggested?

Fair enough! I'll make it clear in a separate issue.

@drbeh drbeh linked an issue Apr 4, 2023 that may be closed by this pull request
wyli
wyli previously approved these changes Apr 4, 2023
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks, it looks good to me now

@wyli wyli dismissed their stale review April 4, 2023 17:39

sorry I thought it was updated

@wyli
Copy link
Contributor

wyli commented Apr 4, 2023

cc @cooperlab @gigony @manthey @shaneahmed from the original feature request in case they are interested in sharing some comments

@drbeh
Copy link
Member Author

drbeh commented Apr 4, 2023

cc @cooperlab @gigony @manthey @shaneahmed from the original feature request in case they are interested in sharing some comments

@wyli as I said we all have already discussed about this in our pathology WG so please be respectful of people's time and do not solicit their comments if not necessary.

@wyli
Copy link
Contributor

wyli commented Apr 4, 2023

@wyli as I said we all have already discussed about this in our pathology WG so please be respectful of people's time and do not solicit their comments if not necessary.

thanks, I couldn't find any documentation/discussion about the proposed interfaces as recommended in rapidsai/cucim#389

@drbeh
Copy link
Member Author

drbeh commented Apr 4, 2023

@wyli as I said we all have already discussed about this in our pathology WG so please be respectful of people's time and do not solicit their comments if not necessary.

thanks, I couldn't find any documentation/discussion about the proposed interfaces as recommended in rapidsai/cucim#389

I believe what you are referring to is not related to this PR. As you correctly spotted, this PR is not addressing #4980, which is related to that cucim feature request. This PR is now closing this feature request #6289. Please let me know if you have any question.

@wyli
Copy link
Contributor

wyli commented Apr 4, 2023

@wyli as I said we all have already discussed about this in our pathology WG so please be respectful of people's time and do not solicit their comments if not necessary.

thanks, I couldn't find any documentation/discussion about the proposed interfaces as recommended in rapidsai/cucim#389

I believe what you are referring to is not related to this PR. As you correctly spotted, this PR is not addressing #4980, which is related to that cucim feature request. This PR is now closing this feature request #6289. Please let me know if you have any question.

I saw that you have edited ticket #6289 to have the opposite meaning, I kind of agree more with the previous edit...

Screenshot 2023-04-04 at 22 22 41

@drbeh
Copy link
Member Author

drbeh commented Apr 5, 2023

I saw that you have edited ticket #6289 to have the opposite meaning, I kind of agree more with the previous edit...

@wyli I have reverted the update to the issue. Please take another look and let me know if all is good with you for this PR to me merged. Thanks

@drbeh drbeh enabled auto-merge (squash) April 5, 2023 18:29
@drbeh drbeh disabled auto-merge April 5, 2023 18:29
@drbeh drbeh enabled auto-merge (squash) April 5, 2023 18:30
@drbeh
Copy link
Member Author

drbeh commented Apr 6, 2023

@wyli @Nic-Ma if there is not any other comments, we should be able to merge this PR. Thanks

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Apr 6, 2023

@wyli @Nic-Ma if there is not any other comments, we should be able to merge this PR. Thanks

I already approved the PR before, @wyli Do you have any other comments?

Thanks.

@wyli
Copy link
Contributor

wyli commented Apr 11, 2023

/build

@drbeh drbeh merged commit 0a29bc1 into Project-MONAI:dev Apr 11, 2023
@drbeh drbeh deleted the wsireader-mpp branch April 11, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WSIReader support for reading by existing power and mpp
4 participants