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

test: add functional vertical flip tests on segmentation mask #5860

Merged

Conversation

federicopozzi33
Copy link
Contributor

No description provided.

@federicopozzi33 federicopozzi33 force-pushed the test/5782-proto-mask-vertical-flip branch from a4da303 to 3577009 Compare April 21, 2022 19:42
@federicopozzi33 federicopozzi33 changed the title feat: add functional vertical flip tests on segmentation mask test: add functional vertical flip tests on segmentation mask Apr 21, 2022
@datumbox datumbox requested review from vfdev-5 and pmeier April 22, 2022 10:12
Copy link
Collaborator

@vfdev-5 vfdev-5 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 the PR @federicopozzi33
I left few comments

test/test_prototype_transforms_functional.py Outdated Show resolved Hide resolved
test/test_prototype_transforms_functional.py Outdated Show resolved Hide resolved
@federicopozzi33
Copy link
Contributor Author

federicopozzi33 commented Apr 22, 2022

@vfdev-5 I saw that in other previous PRs, such as the ones concerning the affine or rotate primitives, you add two tests for check the correctness: one with random segmentation masks and another with a fixed input.

I skipped the one with random segmentation masks for some reasons:

  • I can't figure out how to build the expected mask without calling some "vertical flip" functions.
  • I understand that it represents broader check, but then why should we have both? Maybe because the one with fixed input is more readable and clear to read/understand?

@federicopozzi33 federicopozzi33 requested a review from vfdev-5 April 22, 2022 13:12
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 22, 2022

I can't figure out how to build the expected mask without calling some "vertical flip" functions.

@federicopozzi33 I think it is enough as you did as vflip is not that complicated op.

I understand that it represents broader check, but then why should we have both? Maybe because the one with fixed input is more readable and clear to read/understand?

Affine/rotate ops are configurable and the first correctness check is generating hundreds of subtests running on CPU only. On the other hand, fixed input correctness check is testing the op on a single test-case and on CUDA and CPU. This is a trade-off between CI runtime and exhaustive op checking.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

@federicopozzi33 LGTM, thanks for the PR !

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@datumbox
Copy link
Contributor

@federicopozzi33 Ooops there are conflicts with main branch. Could you please resolve so that we can merge?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 22, 2022

@datumbox I can fix them quickly right now

@vfdev-5 vfdev-5 force-pushed the test/5782-proto-mask-vertical-flip branch from c9589f7 to 30ad075 Compare April 22, 2022 14:54
@vfdev-5 vfdev-5 merged commit e30f9b0 into pytorch:main Apr 22, 2022
facebook-github-bot pushed a commit that referenced this pull request May 6, 2022
#5860)

Summary:
* test: add functional vertical flip tests on segmentation mask

* refactor: improve test readibility

* Update test_prototype_transforms_functional.py

Reviewed By: jdsgomes, NicolasHug

Differential Revision: D36095725

fbshipit-source-id: 12e88faba3cc1f3bf9d9857be2e92b52634155b3

Co-authored-by: Federico Pozzi <federico.pozzi@argo.vision>
Co-authored-by: vfdev <vfdev.5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants