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

update incorrect argument and deprecated function for tifffile.TiffWriter #433

Merged
merged 7 commits into from
Nov 18, 2022

Conversation

JoohyungLee0106
Copy link
Contributor

@JoohyungLee0106 JoohyungLee0106 commented Nov 9, 2022

  1. Both tifffile.TiffWriter.write()[link] and tifffile.TiffWriter.save()[link] receive compression not compress
    -> Current version doesn't work and raises the following error msg:
tifffile TypeError: write() got an unexpected keyword argument 'compress'
  1. tifffile.TiffWriter.write() instead of tifffile.TiffWriter.save(), which has been deprecated [link]

1. ```TiffWriter.write()``` receives ```compression``` instead of ```compress```
2. ```TiffWriter.write()`` instead of ```TiffWriter.save()``
@grlee77
Copy link
Contributor

grlee77 commented Nov 16, 2022

Thanks for fixing the issue @JoohyungLee0106.

I looked into the history of the API change and don't think we may not need to support a fallback code path for older tifffile versions. It looks like the suggestion here should work as long as we use tifffile>=v2020.9.30. Given that this version is from > 2 years ago, that seems okay to me.

We already pinned to a newer version in our test requirements here: https://github.com/rapidsai/cucim/blob/branch-22.12/python/cucim/requirements-test.txt

However, I do see a few files pinned to a specific, older version:
https://github.com/rapidsai/cucim/blob/branch-22.12/docker/requirements-claratrain.txt
https://github.com/rapidsai/cucim/blob/branch-22.12/docker/requirements-jupyter.txt
https://github.com/rapidsai/cucim/blob/branch-22.12/docker/requirements-jupyter-dev.txt
@gigony, do you know of a reason those specific versions were pinned or can we just update them?

@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

1 similar comment
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@grlee77 grlee77 added bug Something isn't working non-breaking Introduces a non-breaking change labels Nov 16, 2022
@grlee77
Copy link
Contributor

grlee77 commented Nov 16, 2022

ok to test

@gigony gigony requested review from a team as code owners November 17, 2022 00:08
@gigony
Copy link
Contributor

gigony commented Nov 17, 2022

@grlee77 those requirement files are not used in anywhere (it was used when cuCIM was released as a separate package but those need to be removed in the future).

Updated this PR to add a test case and update the hard-coded version.

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

grlee77 commented Nov 17, 2022

@gigony, looks like we have to add opencv-python to requirements-test.txt as tiff.py uses cv2.resize

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.

This looks good, assuming CI passes this time around.

gigony and others added 3 commits November 16, 2022 21:14
Conda doesn't have opencv-python package (instead, it has opencv),
so we need to use pip for installing test dependencies.
Signed-off-by: Gigon Bae <gbae@nvidia.com>
run Show resolved Hide resolved
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, everything looks good now.

@grlee77
Copy link
Contributor

grlee77 commented Nov 18, 2022

@gpucibot merge

@grlee77
Copy link
Contributor

grlee77 commented Nov 18, 2022

Thanks again, @JoohyungLee0106. This fix will appear in the 2022.12.00 release (sometime around Dec 7)

@rapids-bot rapids-bot bot merged commit c4604da into rapidsai:branch-22.12 Nov 18, 2022
rapids-bot bot pushed a commit that referenced this pull request Nov 18, 2022
If you want to use no compression method for your converted file, you can set the compression method to None in the converter.

In CLI, you can use `--compression` option and set it to any string except `jpeg`.

E.g.,
```
cucim convert --tile-size 512 --overlap 0 --num-workers 12  --output-filename resize.tiff --compression RAW notebooks/input/TUPAC-TR-467.svs
```

The converter will then convert the file without any compression.

Need to update test cases once #433 is merged.

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

Authors:
  - Gigon Bae (https://github.com/gigony)
  - Gregory Lee (https://github.com/grlee77)

Approvers:
  - Gregory Lee (https://github.com/grlee77)

URL: #443
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants