Add transforms for Digital Pathology by shekhardw · Pull Request #100 · rapidsai/cucim

shekhardw · 2021-09-12T18:18:52Z

Implement transforms for color jitter, image scale intensity, image flip and rotate included for batched RGB images.

This will be used exclusively for digital pathology training pipelines.

closes #105

gigony · 2021-09-13T07:07:03Z

______________________________________________________________________________________________________________________________________ ERROR collecting src/cucim/core/operations/color/tests/test_color_jitter.py _______________________________________________________________________________________________________________________________________
src/cucim/core/operations/color/tests/test_color_jitter.py:6: in <module>
    img = Image.open('1.png')
/home/gbae/.pyenv/versions/3.8.10/envs/cucim-3.8/lib/python3.8/site-packages/PIL/Image.py:2912: in open
    fp = builtins.open(filename, "rb")
E   FileNotFoundError: [Errno 2] No such file or directory: '1.png'
_______________________________________________________________________________________________________________________________________ ERROR collecting src/cucim/core/operations/intensity/tests/test_scaling.py _______________________________________________________________________________________________________________________________________
src/cucim/core/operations/intensity/tests/test_scaling.py:7: in <module>
    img = Image.open('1.png')
/home/gbae/.pyenv/versions/3.8.10/envs/cucim-3.8/lib/python3.8/site-packages/PIL/Image.py:2912: in open
    fp = builtins.open(filename, "rb")
E   FileNotFoundError: [Errno 2] No such file or directory: '1.png'
_________________________________________________________________________________________________________________________________________ ERROR collecting src/cucim/core/operations/spatial/tests/test_flip.py __________________________________________________________________________________________________________________________________________
src/cucim/core/operations/spatial/tests/test_flip.py:6: in <module>
    img = Image.open('1.png')
/home/gbae/.pyenv/versions/3.8.10/envs/cucim-3.8/lib/python3.8/site-packages/PIL/Image.py:2912: in open
    fp = builtins.open(filename, "rb")
E   FileNotFoundError: [Errno 2] No such file or directory: '1.png'
________________________________________________________________________________________________________________________________________ ERROR collecting src/cucim/core/operations/spatial/tests/test_rotate.py _________________________________________________________________________________________________________________________________________
src/cucim/core/operations/spatial/tests/test_rotate.py:6: in <module>
    img = Image.open('1.png')
/home/gbae/.pyenv/versions/3.8.10/envs/cucim-3.8/lib/python3.8/site-packages/PIL/Image.py:2912: in open
    fp = builtins.open(filename, "rb")
E   FileNotFoundError: [Errno 2] No such file or directory: '1.png'
----------------------------------------------------------------------------------------------------------------------------------------------- generated xml file: /home/gbae/repo/cucim/junit-cucim.xml

@shekhardw Please specify the absolute path to load the test data.

(pytest is run in root of the repository so that relative paths are not available).
You may want to use __file__ to get absolute path.

How to reproduce

pushed python/cucim
pip install -e .
popd

./run test python unit skimage # or `./run test_python`

gigony

Please check/auto-format using black/isort.

pip install black
python -m black python/cucim/src/cucim/core/ --check  # check
python -m black python/cucim/src/cucim/core/ # autofix

pip install isort
python -m isort python/cucim/src/cucim/core/ --check  # check
python -m isort python/cucim/src/cucim/core/ # autofix

python/cucim/src/cucim/core/operations/color/__init__.py

python/cucim/src/cucim/core/operations/color/tests/test_color_jitter.py

gigony

Looks good to me.

grlee77

Thanks @shekhardw,
In general the kernels look nice and performance is great, but input validations, docstrings and test cases are very minimal.

For example, it is not always clear from the docstrings:
1.) which dtypes are supported
2.) whether RGB, RGBA and/or arbitrary number of "channels" are supported
3.) whether an additional "batch" dimension is supported.

I have made a number of suggestions, but it could probably use another pass after these are addressed.

I did not review the C++ code for all individual CUDA kernels, but the ones I looked at seemed good to me.

grlee77 · 2021-09-15T17:17:32Z

python/cucim/src/cucim/core/operations/color/jitter.py

+from typing import Any, List, Optional, Tuple
+
+import numpy as np
+import scipy.ndimage as ndimage


Nitpick, but to be consistent with the cucim.skimage and upstream scikit-image, I would use

import scipy.ndimage as ndi

Changes made

python/cucim/src/cucim/core/operations/color/jitter.py

grlee77 · 2021-09-15T19:19:02Z

python/cucim/src/cucim/core/operations/color/tests/test_color_jitter.py

+diff_im = np_output - arr
+diff_total_value = np.abs(np.sum(diff_im))
+
+assert diff_total_value > 0


This very minimal test coverage that only verifies that the kernels run without crashing and that some change to the image has occurred.

Why not place this in a named test function, though, so pytest would report it as a test case?

You could then add additional tests in separate functions. Additional simple tests would be:

a test case with all factors set to 0, verifying that the image is unmodified in that case.

a test with a batch image, e.g. using cupy_arr_batch = cupy.stack((cupy_arr, ) * 8, axis=0). In this case since all elements in the batch are identical we could also verify that for the output.

a test verifying that providing numpy.ndarray input results in a numpy.ndarray output

same output with C or Fortran-ordered inputs

Test cases included for all factors 0, batch image, cupy input, numpy input. Color jitter picks a random sequence to apply 4 operations. Test functions included in all test files.

python/cucim/src/cucim/core/operations/spatial/rotate_and_flip.py

grlee77 · 2021-09-15T20:31:26Z

python/cucim/src/cucim/core/operations/spatial/rotate_and_flip.py

+    R = np.random.RandomState()
+
+    if R.rand() < prob:
+        return image_rotate_90(img, spatial_axis)


Suggested change

return image_rotate_90(img, spatial_axis)

return image_flip(img, spatial_axis)

wrong function!

grlee77 · 2021-09-15T20:35:17Z

python/cucim/src/cucim/core/operations/spatial/tests/test_rotate.py

+
+import cucim.core.operations.spatial as spt
+
+img = Image.open(os.path.join(os.path.abspath(os.path.dirname(__file__)), "1.png"))


What is the source of this dice image? (i.e. is it public domain?)

If there is any rights issue it should be easy enough to just use one of the images bundled with scikit-image.

e.g. for an RGB image:
skimage.data.astronaut() (will need to transpose dimensions to get the color axis from last to first)

test image updated to use astronaut image from scikit-image. Additional test cases included.

gigony

Thanks @grlee77 for the review! @shekhardw would address your comments.

gigony · 2021-09-16T01:57:08Z

python/cucim/src/cucim/core/operations/expose/transform.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from cucim.core.operations.color import color_jitter as color_jitter


Thanks @grlee77 . Let me chime in this question.

We didn't decide what/how to expose APIs from cucim.core.operations and it looks like MONAI's transforms require APIs in a specific form and prone to change.
For this reason, Shekhar and I decided to use cucim.core.operations.expose.transform for exposing APIs needed by MONAI transforms, without messing up cucim.core.operations.xxx namespaces.

We need to discuss how to use cucim.core.operations namespace in detail, as a group, in the next cuCIM bi-weekly meeting.

shekhardw · 2021-09-17T06:57:30Z

@grlee77 @gigony Thanks for your valuable feedback and code review. I have pushed updated test cases. Please let me know if there are any additional suggestions/feedback.

gigony

looks good to me.
please format the updated code so that it can pass CI/CD checks.

@grlee77 Please help review and approval the changes.

gigony · 2021-09-17T16:06:25Z

python/cucim/src/cucim/core/operations/spatial/tests/test_random_flip.py

+    return arr_o
+
+def test_rand_flip_numpy_input():
+


nit: blank line

gigony · 2021-09-17T16:06:38Z

python/cucim/src/cucim/core/operations/spatial/tests/test_random_flip.py

+    assert np.allclose(output,flip_arr)
+
+def test_rand_flip_zero_prob():
+


nit: blank line

please check other parts of test methods that start with a blank line.

corrected and all test functions checked.

gigony · 2021-09-17T16:16:36Z

rerun tests

grlee77

Thanks for addressing the comments @shekhardw. The tests are looking a lot better now and can always be expanded further as needed if issues crop up.

Thanks for also adding the dimension info to the docstrings.

Out of curiosity, what is the source of the 1.png image used in the color tests? (it appears as somewhat random colors). Is there a particular reason for testing with this image?

grlee77 · 2021-09-20T15:50:20Z

python/cucim/src/cucim/core/operations/expose/transform.py

+# limitations under the License.
+
+from cucim.core.operations.color import color_jitter as color_jitter
+from cucim.core.operations.intensity import rand_zoom as rand_zoom


Suggested change

from cucim.core.operations.intensity import rand_zoom as rand_zoom

from cucim.core.operations.intensity import rand_zoom

@grlee77 1.png was created out of random array. I will change it with astronaut image (to be consistent).

grlee77

Please also change import skimage to import skimage.data in the tests as suggested in the recently added comments. I was surprised the former works and I think it may not work across all older scikit-image versions!

It happens to work because there is a from .data import data_dir in skimage.__init__ which apparently loads data into the skimage namespace (see here). It is safer not to rely on this incidental import of data and just import it explicitly.

python/cucim/src/cucim/core/operations/color/tests/test_color_jitter.py

python/cucim/src/cucim/core/operations/intensity/tests/test_rand_zoom.py

python/cucim/src/cucim/core/operations/intensity/tests/test_scaling.py

python/cucim/src/cucim/core/operations/intensity/tests/test_zoom.py

python/cucim/src/cucim/core/operations/spatial/tests/test_flip.py

python/cucim/src/cucim/core/operations/spatial/tests/test_random_flip.py

shekhardw · 2021-09-21T05:57:44Z

Thanks @grlee77 test cases updated.

jakirkham

Thanks Shekhar! 😄

Had a couple questions/suggestion below. For simplicity and clearer discussion focused only on one case of the pattern for discussion, but the same could be applied in other places where that pattern is used

jakirkham · 2021-09-27T21:25:20Z

python/cucim/src/cucim/core/operations/color/jitter.py

+        def get_params(brightness: Optional[List[float]],
+                       contrast: Optional[List[float]],
+                       saturation: Optional[List[float]],
+                       hue: Optional[List[float]]
+                       ) -> Tuple[np.ndarray, Optional[float],
+                                  Optional[float], Optional[float],
+                                  Optional[float]]:


Is it necessary for these to be closures? Or could we define module level functions for these tasks?

@shekhardw, any thoughts on this?

I propose to fix this in a separate MR.

jakirkham · 2021-09-27T21:26:40Z

python/cucim/src/cucim/core/operations/color/jitter.py

+        _logger.error("[cucim] " + str(e), exc_info=True)
+        _logger.info("Error executing color jitter on GPU")
+        raise


Do we want to handle logging within cuCIM? Typically this is the sort of thing that would be configured in a user application (unless for some reason the exception would never be seen)

@shekhardw, any thoughts on this?

I propose to fix this in a separate MR.

jakirkham · 2021-09-27T21:28:59Z

python/cucim/src/cucim/core/operations/color/kernel/cuda_kernel_source.py

+extern "C" {
+__global__ void brightnessjitter_kernel(unsigned char *input_rgb, \


Would it make sense to put these in C file? Might make this is easier to read/lint/test/etc. and also use directly if desired. From the Python side we could always load the file into a str for use as needed

@shekhardw, any thoughts on this?

I propose to fix this in a separate MR.

python/cucim/src/cucim/core/operations/color/tests/test_color_jitter.py

python/cucim/src/cucim/core/operations/expose/tests/test_expose.py

jakirkham · 2021-09-27T21:36:21Z

python/cucim/src/cucim/core/operations/expose/tests/test_expose.py

+def test_exposed_transforms():
+    assert color_jitter is not None
+    assert image_flip is not None
+    assert image_rotate_90 is not None
+    assert scale_intensity is not None
+    assert zoom is not None
+    assert rand_zoom is not None
+    assert rand_image_flip is not None
+    assert rand_image_rotate_90 is not None


Is the goal just to test that these just exist or use them in some way?

just the existence for exposing. Each of the transforms has its own set of test cases.

shekhardw · 2021-09-28T18:17:56Z

@jakirkham Thanks for your valuable feedback. I have updated expose related changes.
Can we address the kernel, logging and closure related changes in another merge request?

jakirkham · 2021-09-29T18:46:10Z

Thanks Shekhar! 😄

There's a couple comments above that may have been missed. Bumped them above. Please let me know your thoughts on those

shekhardw · 2021-09-29T18:50:19Z

Hi @jakirkham, I propose to address the 3 remaining commments on closure, logging and cuda kernel in a separate MR. Are you fine with that?

jakirkham · 2021-09-29T19:10:06Z

In that case could you please raise an issue tracking these?

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

…aling.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

…om_flip.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

…om.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

…nd_zoom.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

…jitter.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

…e.py Co-authored-by: jakirkham <jakirkham@gmail.com>

…jitter.py Co-authored-by: jakirkham <jakirkham@gmail.com>

./src/cucim/core/operations/expose/transform.py:18:1: F40 'cucim.core.operations.spatial.rand_image_rotate_90' imported but unused

jakirkham · 2021-09-29T22:02:40Z

@gpucibot merge

jakirkham · 2021-09-29T22:03:21Z

Thanks Shekhar for the PR and Gigon and Greg for the reviews! 😄

closes #114 Removes the try/except wrappers with logging of errors from the digital pathology transforms added in #100. Also renames a variable `to_cupy` to `to_numpy` instead to reflect its actual function. **For reviewers**: There appear to be many lines changed here, but it is mostly a change in indentation due to removal of `try/except` blocks with `_logger` calls. Also some nested function definitions within `color_jitter` were moved outside of that function. Authors: - Gregory Lee (https://github.com/grlee77) Approvers: - Gigon Bae (https://github.com/gigony) - https://github.com/jakirkham

shekhardw requested a review from a team as a code owner September 12, 2021 18:18

shekhardw assigned gigony and grlee77 Sep 12, 2021

shekhardw added feature request New feature or request non-breaking Introduces a non-breaking change labels Sep 12, 2021

gigony reviewed Sep 13, 2021

View reviewed changes

python/cucim/src/cucim/core/operations/color/__init__.py Outdated Show resolved Hide resolved

python/cucim/src/cucim/core/operations/color/tests/test_color_jitter.py Outdated Show resolved Hide resolved

gigony approved these changes Sep 15, 2021

View reviewed changes

grlee77 requested changes Sep 15, 2021

View reviewed changes

gigony reviewed Sep 16, 2021

View reviewed changes

gigony approved these changes Sep 17, 2021

View reviewed changes

gigony changed the title ~~Initial Transforms for digital pathology~~ [run style] Initial Transforms for digital pathology Sep 17, 2021

gigony changed the title ~~[run style] Initial Transforms for digital pathology~~ Initial Transforms for digital pathology Sep 17, 2021

grlee77 approved these changes Sep 20, 2021

View reviewed changes

gigony changed the title ~~Initial Transforms for digital pathology~~ Add Transforms for digital pathology Sep 21, 2021

gigony changed the title ~~Add Transforms for digital pathology~~ Add transforms for Digital Pathology Sep 21, 2021

grlee77 reviewed Sep 21, 2021

View reviewed changes

gigony requested a review from jakirkham September 27, 2021 17:48

jakirkham reviewed Sep 27, 2021

View reviewed changes

gigony mentioned this pull request Sep 28, 2021

Add Transforms for Digital Pathology #105

Closed

This was referenced Sep 29, 2021

Update DP transforms with module level functions #112

Open

Separate cuda kernels into c files #113

Open

remove logging from DP transforms #114

Closed

shekhardw and others added 20 commits September 29, 2021 13:19

Update python/cucim/src/cucim/core/operations/spatial/rotate_and_flip.py

302447a

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

test cases update and review comments

fedf562

test file cleaned up for rand_rotate90

1f536ab

isort check

50dc754

blank line fix for test cases

9353684

flask8 on color functions

fc488aa

isort on color jitter post flask

712a403

flake8 check applied to all files in core/operations

d4171f8

fixing tests in color jitter and expose

376ce10

cleaning up color test image

4f2816a

Update python/cucim/src/cucim/core/operations/intensity/tests/test_sc…

bc603ff

…aling.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Update python/cucim/src/cucim/core/operations/spatial/tests/test_flip.py

742bfbe

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Update python/cucim/src/cucim/core/operations/spatial/tests/test_rand…

54a6303

…om_flip.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Update python/cucim/src/cucim/core/operations/intensity/tests/test_zo…

47d3699

…om.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Update python/cucim/src/cucim/core/operations/intensity/tests/test_ra…

90c5291

…nd_zoom.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Update python/cucim/src/cucim/core/operations/color/tests/test_color_…

6a8bcd8

…jitter.py Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

update skimage.data import in test cases

3d2bb25

Update python/cucim/src/cucim/core/operations/expose/tests/test_expos…

53e2343

…e.py Co-authored-by: jakirkham <jakirkham@gmail.com>

Update python/cucim/src/cucim/core/operations/color/tests/test_color_…

a616a3b

…jitter.py Co-authored-by: jakirkham <jakirkham@gmail.com>

expose test cases updated per review comments

c77b8f8

gigony force-pushed the dp_transforms branch from bdfd569 to c77b8f8 Compare September 29, 2021 20:20

Ignore flake8 error for transform.py

e697abf

./src/cucim/core/operations/expose/transform.py:18:1: F40 'cucim.core.operations.spatial.rand_image_rotate_90' imported but unused

rapids-bot bot merged commit 629f207 into branch-21.10 Sep 29, 2021

jakirkham deleted the dp_transforms branch September 29, 2021 22:02

quasiben mentioned this pull request Sep 30, 2021

[DOC] Document Transforms for Digital Pathology #119

Closed

gigony mentioned this pull request Oct 5, 2021

[FEA] Add Digital Pathology-related transformations #97

Closed

grlee77 mentioned this pull request May 31, 2022

Remove logging from digital pathology transforms #297

Merged

	return image_rotate_90(img, spatial_axis)
	return image_flip(img, spatial_axis)


		import cucim.core.operations.spatial as spt

		img = Image.open(os.path.join(os.path.abspath(os.path.dirname(__file__)), "1.png"))

		assert np.allclose(output,flip_arr)

		def test_rand_flip_zero_prob():

	from cucim.core.operations.intensity import rand_zoom as rand_zoom
	from cucim.core.operations.intensity import rand_zoom

		extern "C" {
		__global__ void brightnessjitter_kernel(unsigned char *input_rgb, \

Conversation

shekhardw commented Sep 12, 2021 • edited by gigony Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gigony commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gigony left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gigony left a comment

Choose a reason for hiding this comment

Uh oh!

grlee77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gigony left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shekhardw commented Sep 17, 2021

Uh oh!

gigony left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gigony Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gigony commented Sep 17, 2021

Uh oh!

grlee77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grlee77 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shekhardw commented Sep 12, 2021 •

edited by gigony

Loading

gigony commented Sep 13, 2021 •

edited

Loading

gigony Sep 17, 2021 •

edited

Loading