-
Notifications
You must be signed in to change notification settings - Fork 60
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
Conversation
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.
Thanks, Gigon! This looks good to me, I just had one suggestion to modify the TiffGenerator
test utility to test these further.
@grlee77 Added test cases. Thank you for the suggestion! I was able to find a bug and fixed with test cases. |
f56a4cf
to
180c5b4
Compare
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.
Thanks for adding these tests, it looks great.
@gigony, @jakirkham, what makes you guys say that it's not picking up the correct |
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 |
rapidsai#349 (comment) Signed-off-by: Gigon Bae <gbae@nvidia.com>
Thanks @grlee77 , just updated PR if it can resolve the issue. |
rapidsai#349 (comment) 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>
Signed-off-by: Gigon Bae <gbae@nvidia.com>
rapidsai#349 (comment) Signed-off-by: Gigon Bae <gbae@nvidia.com>
f44dbb4
to
88c5f11
Compare
Failing Build Somehow, In failing build, libcucim package is updated by the following script(command) which causes failure on test.
@ajschmidt8 @jakirkham @grlee77 https://docs.conda.io/projects/conda/en/latest/commands/install.html |
Signed-off-by: Gigon Bae <gbae@nvidia.com>
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 |
@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>
I see that your commit adding update: I just checked locally and I can see the same failure when using |
The following two items in the
Let me see if we could use |
Our options appear to be: |
Option 1 is maybe best for now and open an issue to transition to option 2. I did try changing to use |
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 Here is a PR that fixes things to work with the most recent |
Thank you @grlee77 for finding the issue and providing the PR to fix it!! |
@gpucibot merge |
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 |
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.
Just a note, this could indicates some other issue we are running into.
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.
Filed as issue ( #382 )
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 onmicrometer
.spacing_test.py
Test install
Address #333
Signed-off-by: Gigon Bae gbae@nvidia.com