Skip to content

Raise import error for openslide and cuimage#1699

Merged
bhashemian merged 2 commits intoProject-MONAI:masterfrom
bhashemian:raise_wsireader_import_error
Mar 7, 2021
Merged

Raise import error for openslide and cuimage#1699
bhashemian merged 2 commits intoProject-MONAI:masterfrom
bhashemian:raise_wsireader_import_error

Conversation

@bhashemian
Copy link
Copy Markdown
Member

Description

In order for WSIReader to be used in LoadImage, optional import error in the __init__ of WSIReader was skipped, so a secondary exception was raised instead, which is not informative and does not help the user to know the cause. This PR fixes this issue by raising the appropriate import error when WSIReader.read is being called.

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

@bhashemian bhashemian requested review from Nic-Ma and wyli March 5, 2021 17:42
@bhashemian bhashemian enabled auto-merge (squash) March 5, 2021 17:43
@bhashemian
Copy link
Copy Markdown
Member Author

/black

Signed-off-by: Behrooz <[email protected]>
data: file name or a list of file names to read.

"""
if (self.reader_lib == "openslide") and (not has_osl):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could have these logics even earlier for example between line 646 and 652?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually no, Nic is instantiating this class in LoadImage, so the error shouldn't be raised in __init__ (it used to be raise without having has_osl and has_cux). The first place that it can be raised here.

I agree that the best place would be when the class is instantiating but for that purpose LoadImage need to be changed:

self.readers: List[ImageReader] = [ITKReader(), NumpyReader(), PILReader(), NibabelReader(), WSIReader()]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sure you're right!

(@Nic-Ma I'm approving this for now, perhaps we need another iteration to revise the base API?)

Copy link
Copy Markdown
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!

@bhashemian bhashemian merged commit 66811fe into Project-MONAI:master Mar 7, 2021
@bhashemian bhashemian deleted the raise_wsireader_import_error branch March 22, 2021 15:40
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.

2 participants