-
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
Bug/fix slide bounds in TiffFile #196
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sinberlin2
reviewed
Nov 20, 2023
I did, you can look at the test coverage On Nov 20, 2023, at 1:06 PM, Shannon Doyle ***@***.***> wrote:
@sinberlin2 commented on this pull request.
In dlup/backends/tifffile_backend.py:
@@ -47,9 +47,9 @@ def __parse_tifffile(self) -> None:
for idx, page in enumerate(self._image.pages):
# Remove channel dimension and swap rows and columns
if len(page.shape) == 3:
- self._shapes.append((page.shape[1], page.shape[2]))
+ self._shapes.append((page.shape[1], page.shape[0]))
Did you test this? In the documentation of tifffile it says that a Tiffpage has Shape ([depth,] length, width)
https:/cgohlke/tifffile/blob/master/tifffile/tifffile.py#L7729
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
sinberlin2
reviewed
Nov 20, 2023
sinberlin2
reviewed
Nov 20, 2023
sinberlin2
reviewed
Nov 20, 2023
I think this is needed because the array swaps it with respect to the image (x,y) versus (rows, cols)On Nov 20, 2023, at 1:45 PM, Shannon Doyle ***@***.***> wrote:
@sinberlin2 commented on this pull request.
In tests/backends/test_backends.py:
+ original_array = np.asarray(test_image)
+ properties = tiff_slide.properties
+ tile_size = (properties["tifffile.level[0].TileWidth"], properties["tifffile.level[0].TileLength"])
+ num_levels = int(np.ceil(np.log2(np.asarray(size[::-1]) / np.asarray(tile_size))).min()) + 1
+
+ # Let's try to read outside of the slide levels. This should give a RuntimeError
+ with pytest.raises(RuntimeError):
+ tiff_slide.read_region((0, 0), num_levels + 1, (1, 1))
+
+ # We need to check a few regions to make sure the backend is working correctly
+ # 1. Check the whole image
+ # 2. Check the top right corner
+ # 3. Check the bottom left corner
+
+ # Let's create a list of origins and sizes.
+ _size = size[::-1]
why does the size order need to be reversed? Can we not just specify the size when we create the tiffile. It is also not really clear in what order the original size specification is. I would assume it is [w,h]. Then it only needs to be reversed when writing the tiffile.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
To make it consistent with _image.pyOn Nov 20, 2023, at 1:51 PM, Shannon Doyle ***@***.***> wrote:
@sinberlin2 commented on this pull request.
In dlup/annotations.py:
@@ -757,8 +757,8 @@ def read_region(
Parameters
----------
- coordinates: np.ndarray or tuple
- region_size : np.ndarray or tuple
+ location: np.ndarray or tuple
why the change to location?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
It is possible, I just want to avoid that accidentally I am reading a region which is by accident correct (like the top left corner will typically be)On Nov 20, 2023, at 1:35 PM, Shannon Doyle ***@***.***> wrote:
@sinberlin2 commented on this pull request.
In tests/backends/test_backends.py:
+class TestBackends:
+ def read_region_and_properties_asserts(self, tiff_slide, openslide_slide, mode, size, mpp, pyramid, test_image):
+ original_array = np.asarray(test_image)
+ properties = tiff_slide.properties
+ tile_size = (properties["tifffile.level[0].TileWidth"], properties["tifffile.level[0].TileLength"])
+ num_levels = int(np.ceil(np.log2(np.asarray(size[::-1]) / np.asarray(tile_size))).min()) + 1
+
+ # Let's try to read outside of the slide levels. This should give a RuntimeError
+ with pytest.raises(RuntimeError):
+ tiff_slide.read_region((0, 0), num_levels + 1, (1, 1))
+
+ # We need to check a few regions to make sure the backend is working correctly
+ # 1. Check the whole image
+ # 2. Check the top right corner
+ # 3. Check the bottom left corner
+
Why is this the top right and bottom left corner? It seems to me that the code corresponds to 2. the top left corner and 3. the middle
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
sinberlin2
previously approved these changes
Nov 20, 2023
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.
The tests work. I added minor comments about naming.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The TiffFile had the axis swapped (height, width) instead of width, height. This fixes #187.
Also took the opportunity to change a few stylistic things.