-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allow to overwrite mpp #128
Conversation
- `overwrite_mpp=(mpp_x, mpp_y)` can now be added to the `SlideImage` and the `TiledROISlideImageDataset` constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; some minor comments.
dlup/_image.py
Outdated
|
||
if self._wsi.spacing is None: | ||
raise UnsupportedSlideError( | ||
f"{identifier} has not parsable spacing and not explicitly set in the `overwrite_mpp` parameter." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment not entirely grammatical.
mpp = np.array([mpp_y, mpp_x]) | ||
self._spacings = [tuple(mpp * downsample) for downsample in self.level_downsamples] | ||
self._spacings = [cast(tuple[float, float], tuple(mpp * downsample)) for downsample in self.level_downsamples] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._spacings
: I'd prefer to always add the unit of measurement in the name of any quantitative variable; e.g. self._spacings_mpp
in this case. Can save a lot of headaches and difficult-to-spot bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but in this case I don't see too much discrepancy. What other spacing would we have?
overwrite_mpp=(mpp_x, mpp_y)
can now be added to theSlideImage
and theTiledROISlideImageDataset
constructors.