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

Enable FutureWarnings/DeprecationWarnings as errors #734

Merged
merged 7 commits into from
May 14, 2024

Conversation

mroeschke
Copy link
Contributor

@mroeschke mroeschke commented May 1, 2024

As part of rapidsai/build-planning#26, enabling Python test failures for FutureWarnings and DeprecationWarnings

@mroeschke mroeschke requested a review from a team as a code owner May 1, 2024 21:00
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Matthew! 🙏

Had a couple questions below

@@ -73,11 +73,12 @@ def svs2tif(
logger.info(" num_workers: %d", num_workers)
logger.info(" compression: %s", compression)
logger.info(" output filename: %s", output_filename)

compressionargs = None
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to be user facing? Or do we want to keep this internal for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we make it user facing, we may need to add some additional validation on the input. I would be fine with just keeping it internal for now.

@gigony do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

@grlee77 @jakirkham
It seems appropriate to update it now since this API change has been in effect since July 2022.
https://github.com/cgohlke/tifffile/blame/ec6be6289db1b8b7327bb98816a764c95b9b7299/README.rst#L519

Copy link
Member

Choose a reason for hiding this comment

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

@grlee77 were there particular thoughts you had on what input validation would look like?

Wonder if tifffile's own validation would work for us

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep internal for now and merge this change as it is.

python/cucim/tests/util/gen_tiff.py Show resolved Hide resolved
python/cucim/tests/util/gen_tiff.py Outdated 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 for making this changes, I found one comment where bdtr wasn't updated to betainc. Otherwise it looks good to me.

python/cucim/src/cucim/skimage/_vendored/_pearsonr.py Outdated Show resolved Hide resolved
@@ -73,11 +73,12 @@ def svs2tif(
logger.info(" num_workers: %d", num_workers)
logger.info(" compression: %s", compression)
logger.info(" output filename: %s", output_filename)

compressionargs = None
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make it user facing, we may need to add some additional validation on the input. I would be fine with just keeping it internal for now.

@gigony do you have a preference?

mroeschke and others added 3 commits May 9, 2024 13:27
Co-authored-by: Gregory Lee <grlee77@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
@gigony gigony added non-breaking Introduces a non-breaking change maintenance labels May 14, 2024
@jakirkham jakirkham added improvement Improves an existing functionality maintenance and removed maintenance labels May 14, 2024
@jakirkham jakirkham added this to the v24.06.00 milestone May 14, 2024
@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 633aa35 into rapidsai:branch-24.06 May 14, 2024
45 checks passed
@mroeschke mroeschke deleted the test/wae branch May 14, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality maintenance non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants