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

Support resolution and spacing metadata #349

Merged
merged 8 commits into from
Aug 3, 2022

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Jul 27, 2022

Add 'tiff.resolution_unit' and 'tiff.x_resolution', 'tiff.y_resolution' metadata to the CuImage object.
(See https://www.awaresystems.be/imaging/tiff/tifftags/resolutionunit.html)

If resolution_unit is not 1 (1 = No absolute unit of measurement), spacing_units is ['micrometer', 'micrometer', 'color'] and spacing unit is calculated based on micrometer.

spacing_test.py

from pprint import pprint
from cucim import CuImage
img = CuImage("TCGA-18-3406-01Z-00-DX1_tissue.tif")
pprint(img.metadata, indent=2, compact=True)
❯ python spacing_test.py
{ 'cucim': { 'associated_images': [],
             'channel_names': ['R', 'G', 'B', 'A'],
             'coord_sys': 'LPS',
             'dims': 'YXC',
             'direction': [[1.0, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]],
             'dtype': {'bits': 8, 'code': 1, 'lanes': 1},
             'ndim': 3,
             'origin': [0.0, 0.0, 0.0],
             'path': '/home/gbae/Downloads/TCGA-18-3406-01Z-00-DX1_tissue.tif',
             'resolutions': { 'level_count': 6,
                              'level_dimensions': [ [16034, 10970],
                                                    [8017, 5485], [4008, 2742],
                                                    [2004, 1371], [1002, 685],
                                                    [501, 342]],
                              'level_downsamples': [ 1.0, 2.0,
                                                     4.000614166259766,
                                                     8.001228332519531,
                                                     16.008296966552734,
                                                     32.040008544921875],
                              'level_tile_sizes': [ [512, 512], [512, 512],
                                                    [512, 512], [512, 512],
                                                    [512, 512], [512, 512]]},
             'shape': [10970, 16034, 4],
             'spacing': [2.0159945487976074, 2.0159945487976074, 1.0],
             'spacing_units': ['micrometer', 'micrometer', 'color'],
             'typestr': '|u1'},
  'tiff': { 'model': '',
            'resolution_unit': 'centimeter',
            'software': '',
            'x_resolution': 4960.3310546875,
            'y_resolution': 4960.3310546875}}

Test install

python -m pip install --force-reinstall --extra-index-url https://test.pypi.org/simple/ cucim==0.0.333

Address #333

Signed-off-by: Gigon Bae gbae@nvidia.com

@gigony gigony added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 27, 2022
@gigony gigony added this to the v22.08.00 milestone Jul 27, 2022
@gigony gigony requested review from a team as code owners July 27, 2022 10:26
@gigony gigony self-assigned this Jul 27, 2022
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks, Gigon! This looks good to me, I just had one suggestion to modify the TiffGenerator test utility to test these further.

@gigony
Copy link
Contributor Author

gigony commented Jul 28, 2022

@grlee77 Added test cases. Thank you for the suggestion! I was able to find a bug and fixed with test cases.

@gigony gigony force-pushed the update_spacing branch 4 times, most recently from f56a4cf to 180c5b4 Compare August 1, 2022 18:02
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests, it looks great.

@gigony
Copy link
Contributor Author

gigony commented Aug 1, 2022

Hi @ajschmidt8

It seems like CI/CD is not using libcucim library built with the change in the PR when both C++ and Python code is changed.

It's weird because the same tests (such as 'testimg_tiff_stripe_32x24_16_jpeg') are passing with other configurations
https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cucim/job/prb/job/cucim-gpu-test/CUDA=11.2,GPU_LABEL=driver-495,LINUX_VER=ubuntu18.04,PYTHON=3.9/375/console
(gpuCI/cucim/gpu/cuda/11.2/driver-495/python/3.9/ubuntu18.04 — Build #375 succeeded in 57 min)

Do you have any idea on this?

@ajschmidt8
Copy link
Member

@gigony, @jakirkham, what makes you guys say that it's not picking up the correct libcucim packages? When I look at the logs below, for this run, it does seem to indicate that it's installing libcucim and cucim from the filesystem.

image

@grlee77
Copy link
Contributor

grlee77 commented Aug 2, 2022

@gigony, @jakirkham, what makes you guys say that it's not picking up the correct libcucim packages?

I think it was just that these tests pass when running them locally.

I initially saw a few failures like on CI, but after running
./run build_local clean_cache
and then rebuilding they pass.

gigony added a commit to gigony/cucim that referenced this pull request Aug 2, 2022
rapidsai#349 (comment)
Signed-off-by: Gigon Bae <gbae@nvidia.com>
@gigony
Copy link
Contributor Author

gigony commented Aug 2, 2022

@gigony, @jakirkham, what makes you guys say that it's not picking up the correct libcucim packages?

I think it was just that these tests pass when running them locally.

I initially saw a few failures like on CI, but after running ./run build_local clean_cache and then rebuilding they pass.

Thanks @grlee77 , just updated PR if it can resolve the issue.

gigony added a commit to gigony/cucim that referenced this pull request Aug 2, 2022
rapidsai#349 (comment)
Signed-off-by: Gigon Bae <gbae@nvidia.com>
@gigony gigony requested a review from a team as a code owner August 2, 2022 18:05
@gigony
Copy link
Contributor Author

gigony commented Aug 2, 2022

@grlee77 It is still failing. Now I am trying to disable cache (not sure if this is the correct way of disabling cache or not): 777b021

Signed-off-by: Gigon Bae <gbae@nvidia.com>
Signed-off-by: Gigon Bae <gbae@nvidia.com>
Signed-off-by: Gigon Bae <gbae@nvidia.com>
Signed-off-by: Gigon Bae <gbae@nvidia.com>
gigony added a commit to gigony/cucim that referenced this pull request Aug 2, 2022
rapidsai#349 (comment)
Signed-off-by: Gigon Bae <gbae@nvidia.com>
@gigony gigony force-pushed the update_spacing branch 2 times, most recently from f44dbb4 to 88c5f11 Compare August 2, 2022 19:37
@gigony
Copy link
Contributor Author

gigony commented Aug 3, 2022

Working Build
https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cucim/job/prb/job/cucim-gpu-test/CUDA=11.2,GPU_LABEL=driver-495,LINUX_VER=ubuntu18.04,PYTHON=3.9/388/consoleFull

image

Failing Build
https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cucim/job/prb/job/cucim-gpu-test/CUDA=11.0,GPU_LABEL=driver-450,LINUX_VER=centos7,PYTHON=3.8/388/consoleFull
image

Somehow, In failing build, libcucim package is updated by the following script(command) which causes failure on test.

13:21:30 ++ ./run test python all
13:21:30 2022-08-02 20:21:30 $ conda install -c conda-forge -y --file /workspace/python/cucim/requirements-test.txt

@ajschmidt8 @jakirkham @grlee77
Let me try to add --freeze-installed (or --no-update-deps) options to the command

https://docs.conda.io/projects/conda/en/latest/commands/install.html

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

Let me try to add --freeze-installed (or --no-update-deps) options to the command

Interesting, great detective work Gigon!

Also related to that requirements text file, it fails in my local environment (I think those exact pinnings aren't available for Python 3.9 or something). If I relaxed the pinnings from== to >= then everything worked as expected. What do you think about making that change?

@gigony
Copy link
Contributor Author

gigony commented Aug 3, 2022

Also related to that requirements text file, it fails in my local environment (I think

@grlee77 I think that is a good idea. Let me update the requirements file(https://github.com/rapidsai/cucim/blob/branch-22.08/python/cucim/requirements-test.txt).

This solves test issues when installing locally.
: rapidsai#349

Signed-off-by: Gigon Bae <gbae@nvidia.com>
@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

I see that your commit adding --freeze-installed passed, but then the follow up relaxing the pinnings is back to a few test failures with all sizes being 1 again! I checked the log and libcucim looks like it is installed as expected (i.e. not from rapidsai-nightly).

update: I just checked locally and I can see the same failure when using tifffile 2022.7.31 (but passes with my previously installed version of 2022.5.4). I will see if I can identify a fix aside from putting an upper bound on tifffile again.

@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

The following two items in the tifffile 2022.7.28 changelog look like the likely culprit:

Standardize resolution parameter and property.
Deprecate third resolution argument on write (use resolutionunit).

Let me see if we could use resolutionunit in older releases as well

@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

Our options appear to be:
1.) keep the code as-is, but pin to tifffile>=2021.7.2,<=2022.5.4
2.) change the test functions to pass 'INCHES' or "CENTIMETER' to a resolutionunit kwarg instead of as the third item in the resolution tuple. This appears to have been an abrupt recent change and would require pinning to tifffile>=2022.7.28
3.) Explicitly check the tifffile version in the test code and adapt the kwargs accordingly.

@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

Option 1 is maybe best for now and open an issue to transition to option 2. I did try changing to use resolutionunit but was still encountering 1 failure. Also, it looks like the 3-tuple resolution is still supposed to work. There is some warning code commented out..., but the code path is still there.

@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

The final test case was failing was due to a change where resolutionunit gets set to INCH if it is not provided whereas previously it would be None. To get an actual unitless case, we have to set resolutionunit=tifffile.RESUNIT.NONE. I am not sure if that was an intentional breaking change, but can check.

Here is a PR that fixes things to work with the most recent tifffile: gigony#4. I think since it is only a test requirement we are free to bump to this recent version?

@gigony
Copy link
Contributor Author

gigony commented Aug 3, 2022

The final test case was failing was due to a change where resolutionunit gets set to INCH if it is not provided whereas previously it would be None. To get an actual unitless case, we have to set resolutionunit=tifffile.RESUNIT.NONE. I am not sure if that was an intentional breaking change, but can check.

Here is a PR that fixes things to work with the most recent tifffile: gigony#4. I think since it is only a test requirement we are free to bump to this recent version?

Thank you @grlee77 for finding the issue and providing the PR to fix it!!
I have merged your PR to this branch. Let's see if this PR is passing now.
Thank you!

@grlee77
Copy link
Contributor

grlee77 commented Aug 3, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 39f7c01 into rapidsai:branch-22.08 Aug 3, 2022
run
Comment on lines -809 to 812
run_command conda install -c conda-forge -y \
# https://github.com/rapidsai/cucim/pull/349#issuecomment-1203335731
# Do not update or change already-installed dependencies.
run_command conda install -c conda-forge -y --freeze-installed \
--file ${TOP}/python/cucim/requirements-test.txt
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, this could indicates some other issue we are running into.

Copy link
Member

Choose a reason for hiding this comment

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

Filed as issue ( #382 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants