adopt torch.testing.assert_close #1031

pmeier · 2021-05-24T18:50:56Z

Closes #1029. torch.testing.assert_close will only be shipped with torch==1.9. Thus, this PR should not be merged before.

All files except for test/utils.py are 1-to-1 ports and probably do not need special review, but I let you be the judge of that.

test/utils.py

shijianjian · 2021-05-25T01:14:01Z

Just one quick question, would it break the backward compatibility in the CI?

pmeier · 2021-05-25T07:06:39Z

Just one quick question, would it break the backward compatibility in the CI?

No, it won't. I've backported it using torch.testing.assert_allclose in case torch.testing.assert_close is not available.

edgarriba · 2021-05-25T13:14:36Z

thanks @pmeier ! this is a great contribution from the pytorch team :)

Borda · 2021-06-01T12:21:57Z

No, it won't. I've backported it using torch.testing.assert_allclose in case torch.testing.assert_close is not available.

so is torch.testing.assert_close missing in torch<1.9?

pmeier · 2021-06-01T12:47:57Z

so is torch.testing.assert_close missing in torch<1.9?

Yes. It will be shipped the first time with torch==1.9.0.

Borda · 2021-06-01T13:04:14Z

so is torch.testing.assert_close missing in torch<1.9?

Yes. It will be shipped the first time with torch==1.9.0.

co can you run all these updated tests in past PT 1.7 for example?

pmeier · 2021-06-01T13:24:22Z

The CI already does that, doesn't it?

Borda · 2021-06-01T13:27:36Z

The CI already does that, doesn't it?

that is something that makes me suspicious, how it can use a future function which is not existing in that codebase? 😕

well, the multi-version is not running on PR so that is why it is passing...

kornia/.github/workflows/tests_cpu_versions.yml

Lines 3 to 9 in f182c58

    
           on: 
        
             push: 
        
              branches: 
        
                - master 
        
             # pull_request: 
        
             schedule: 
        
               - cron: "0 4 * * *"

edgarriba · 2021-06-01T13:33:23Z

@pmeier @Borda we disabled mult-version in the CI to lighten the load

pmeier · 2021-06-01T13:39:05Z

@Borda See #1031 (comment)

torch.testing.assert_close will only be shipped with torch==1.9. Thus, this PR should not be merged before.

I've just prepared the PR so this is ready when kornia officially supports torch==1.9. I can confirm that it runs fine with the pre-release, but we need for CI to confirm that as soon as it is able to.

edgarriba · 2021-06-01T13:45:01Z

I guess that the point of @Borda is that will this PR won't have backward compatibility

pmeier · 2021-06-01T14:01:30Z

Why wouldn't it? CI is green right now for all the older versions.

edgarriba · 2021-06-01T14:53:00Z

@pmeier you are right. Was confused by a moment with @Borda comment. The PR runs on a lighter version of the full version matrix. See: https://github.com/kornia/kornia/blob/master/.github/workflows/tests_cpu.yml#L50

@Borda what was your point here ? 🤔

Borda · 2021-06-01T15:08:05Z

I've just prepared the PR so this is ready when kornia officially supports torch==1.9.

But we cannot use it till Kornia will set 1.9 as min version...

@Borda what was your point here ? 🤔

As mentioned, the CI for PR will pass but not on mater, and you can't test any older PT versions anymore...

The PR runs on a lighter version of the full version matrix.

So how is it possible that your tests are using function that shall not exist in the particular PT version?

pmeier · 2021-06-02T06:56:07Z

@Borda before we continue this cycle, please have a look at the file test/utils.py that will be added by this PR.

I've just prepared the PR so this is ready when kornia officially supports torch==1.9.

But we cannot use it till Kornia will set 1.9 as min version...

We can use it for all CI runs that run on torch>=1.9. Currently, there is no such run, but there will be in the near future.

@Borda what was your point here ? thinking

As mentioned, the CI for PR will pass but not on mater, and you can't test any older PT versions anymore...

Of course it will pass on master. I did not touch the minimum requirement of torch and the CI is green.

The PR runs on a lighter version of the full version matrix.

So how is it possible that your tests are using function that shall not exist in the particular PT version?

The tests aren't doing that. As I have explained in #1031 (comment) I've added a minimal backport of torch.testing.assert_close in case it is not available. So all our current tests are using this backport.

Borda · 2021-06-02T09:25:58Z

@Borda before we continue this cycle, please have a look at the file test/utils.py that will be added by this PR.

yeah, I missed this part, and in fact, I was just backing about this point, sorry for the confusion... 🐰

edgarriba · 2021-06-02T09:44:19Z

@pmeier @Borda actually forgot that we run against nightlies in daily basis and everything seems fine
https://github.com/kornia/kornia/runs/2724828690?check_suite_focus=true

pmeier · 2021-06-16T11:58:12Z

@edgarriba @Borda please let me know when the CI includes torch>=1.9 so I can update the PR.

edgarriba · 2021-06-23T13:16:36Z

@pmeier #1120 all yours !

edgarriba · 2021-06-25T10:50:01Z

@pmeier I think you need to rebase so that can trigger 1.9 tests

pmeier · 2021-06-25T10:51:11Z

Yes, but I didn't have time yet to do so. I'll request another review when the PR is ready.

for more information, see https://pre-commit.ci

pmeier · 2021-06-25T18:02:53Z

One thing that is different from the old assert_allclose to the new assert_close is the default tolerance. We run some diagnostics and set the new tolerances a little stricter than the old ones. The current adoption reuses the old tolerances. If we were to use the new ones, these tests would fail.

test/augmentation/test_augmentation_mix.py::TestRandomCutMix::test_random_mixup_beta0
test/augmentation/test_random_generator.py::TestRandomPerspectiveGen::test_random_gen
test/augmentation/test_random_generator.py::TestRandomPerspectiveGen::test_same_on_batch
test/augmentation/test_random_generator.py::TestRandomRotationGen::test_random_gen
test/augmentation/test_random_generator.py::TestRandomRotationGen::test_random_gen
test/augmentation/test_random_generator.py::TestRandomRotationGen::test_same_on_batch
test/color/test_hls.py::TestRgbToHls::test_unit
test/color/test_hls.py::TestHlsToRgb::test_unit
test/color/test_hsv.py::TestRgbToHsv::test_unit
test/enhance/test_adjust.py::TestAdjustSaturation::test_saturation_one
test/enhance/test_adjust.py::TestAdjustSaturation::test_saturation_one_batch
test/enhance/test_adjust.py::TestAdjustHue::test_hue_one
test/enhance/test_adjust.py::TestAdjustHue::test_hue_one_batch
test/feature/test_matching.py::TestMatchSMNN::test_matching1
test/filters/test_sobel.py::TestSobel::test_magnitude
test/filters/test_sobel.py::TestSobel::test_magnitude
test/geometry/transform/test_imgwarp.py::TestWarpPerspective::test_crop_center_resize
test/geometry/transform/crop/test_crop3d.py::TestCropAndResize3D::test_crop
test/geometry/transform/crop/test_crop3d.py::TestCropAndResize3D::test_crop_batch
test/integration/test_conversions.py::TestAngleOfRotations::test_matrix_angle
test/utils/test_metrics.py::TestConfusionMatrix::test_three_classes_normalized

@edgarriba could you have a look if there is anything special to these tests? If not can I just crank up the tolerances in there and use the new defaults everywhere else?

kornia/testing/__init__.py

edgarriba · 2021-06-26T10:10:15Z

One thing that is different from the old assert_allclose to the new assert_close is the default tolerance. We run some diagnostics and set the new tolerances a little stricter than the old ones. The current adoption reuses the old tolerances. If we were to use the new ones, these tests would fail.

test/augmentation/test_augmentation_mix.py::TestRandomCutMix::test_random_mixup_beta0
test/augmentation/test_random_generator.py::TestRandomPerspectiveGen::test_random_gen
test/augmentation/test_random_generator.py::TestRandomPerspectiveGen::test_same_on_batch
test/augmentation/test_random_generator.py::TestRandomRotationGen::test_random_gen
test/augmentation/test_random_generator.py::TestRandomRotationGen::test_random_gen
test/augmentation/test_random_generator.py::TestRandomRotationGen::test_same_on_batch
test/color/test_hls.py::TestRgbToHls::test_unit
test/color/test_hls.py::TestHlsToRgb::test_unit
test/color/test_hsv.py::TestRgbToHsv::test_unit
test/enhance/test_adjust.py::TestAdjustSaturation::test_saturation_one
test/enhance/test_adjust.py::TestAdjustSaturation::test_saturation_one_batch
test/enhance/test_adjust.py::TestAdjustHue::test_hue_one
test/enhance/test_adjust.py::TestAdjustHue::test_hue_one_batch
test/feature/test_matching.py::TestMatchSMNN::test_matching1
test/filters/test_sobel.py::TestSobel::test_magnitude
test/filters/test_sobel.py::TestSobel::test_magnitude
test/geometry/transform/test_imgwarp.py::TestWarpPerspective::test_crop_center_resize
test/geometry/transform/crop/test_crop3d.py::TestCropAndResize3D::test_crop
test/geometry/transform/crop/test_crop3d.py::TestCropAndResize3D::test_crop_batch
test/integration/test_conversions.py::TestAngleOfRotations::test_matrix_angle
test/utils/test_metrics.py::TestConfusionMatrix::test_three_classes_normalized

@edgarriba could you have a look if there is anything special to these tests? If not can I just crank up the tolerances in there and use the new defaults everywhere else?

@pmeier probably it's worth opening an issue and mention that we should take a look at this tests and their implementations.

* adopt torch.testing.assert_close * use torch.testing.assert_close * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add TODO Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

adopt torch.testing.assert_close

9041c42

pmeier commented May 24, 2021

View reviewed changes

test/utils.py Outdated Show resolved Hide resolved

edgarriba added enhancement 🚀 Improvement over an existing functionality 2 Priority 2 😅 Medium priority code heatlh 💊 Improvement the package code health labels May 31, 2021

pmeier and others added 3 commits June 25, 2021 19:39

Merge branch 'master' into adopt-assert-close

62df5f8

use torch.testing.assert_close

cf1e9e5

[pre-commit.ci] auto fixes from pre-commit.com hooks

dc4cfb5

for more information, see https://pre-commit.ci

pmeier marked this pull request as ready for review June 25, 2021 18:02

pmeier requested review from ducha-aiki, edgarriba, lferraz and shijianjian as code owners June 25, 2021 18:02

edgarriba approved these changes Jun 26, 2021

View reviewed changes

edgarriba reviewed Jun 26, 2021

View reviewed changes

kornia/testing/__init__.py Show resolved Hide resolved

edgarriba added 1 Priority 1 🚨 High priority and removed 2 Priority 2 😅 Medium priority labels Jun 26, 2021

add TODO

d6ab476

edgarriba merged commit 04a6ad1 into kornia:master Jun 27, 2021

pmeier deleted the adopt-assert-close branch June 27, 2021 12:57

pmeier mentioned this pull request Jun 28, 2021

New default tolerances makes some tests fail #1134

Open

pmeier mentioned this pull request Aug 20, 2021

replace pytorch_testing_utils with native PyTorch solution pystiche/pystiche#524

Open

pmeier mentioned this pull request Nov 3, 2022

The latest pytorch nightly has removed torch.testing.assert_allclose #1981

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adopt torch.testing.assert_close #1031

adopt torch.testing.assert_close #1031

pmeier commented May 24, 2021 •

edited

shijianjian commented May 25, 2021

pmeier commented May 25, 2021

edgarriba commented May 25, 2021

Borda commented Jun 1, 2021

pmeier commented Jun 1, 2021

Borda commented Jun 1, 2021

pmeier commented Jun 1, 2021

Borda commented Jun 1, 2021 •

edited

edgarriba commented Jun 1, 2021

pmeier commented Jun 1, 2021 •

edited

edgarriba commented Jun 1, 2021

pmeier commented Jun 1, 2021

edgarriba commented Jun 1, 2021 •

edited

Borda commented Jun 1, 2021 •

edited

pmeier commented Jun 2, 2021

Borda commented Jun 2, 2021

edgarriba commented Jun 2, 2021

pmeier commented Jun 16, 2021

edgarriba commented Jun 23, 2021

edgarriba commented Jun 25, 2021

pmeier commented Jun 25, 2021

pmeier commented Jun 25, 2021

edgarriba commented Jun 26, 2021

adopt torch.testing.assert_close #1031

adopt torch.testing.assert_close #1031

Conversation

pmeier commented May 24, 2021 • edited

shijianjian commented May 25, 2021

pmeier commented May 25, 2021

edgarriba commented May 25, 2021

Borda commented Jun 1, 2021

pmeier commented Jun 1, 2021

Borda commented Jun 1, 2021

pmeier commented Jun 1, 2021

Borda commented Jun 1, 2021 • edited

edgarriba commented Jun 1, 2021

pmeier commented Jun 1, 2021 • edited

edgarriba commented Jun 1, 2021

pmeier commented Jun 1, 2021

edgarriba commented Jun 1, 2021 • edited

Borda commented Jun 1, 2021 • edited

pmeier commented Jun 2, 2021

Borda commented Jun 2, 2021

edgarriba commented Jun 2, 2021

pmeier commented Jun 16, 2021

edgarriba commented Jun 23, 2021

edgarriba commented Jun 25, 2021

pmeier commented Jun 25, 2021

pmeier commented Jun 25, 2021

edgarriba commented Jun 26, 2021

pmeier commented May 24, 2021 •

edited

Borda commented Jun 1, 2021 •

edited

pmeier commented Jun 1, 2021 •

edited

edgarriba commented Jun 1, 2021 •

edited

Borda commented Jun 1, 2021 •

edited