Skip to content

Conversation

puhuk
Copy link
Contributor

@puhuk puhuk commented Nov 4, 2021

To resolve issue #4818
Add assert function and logic after checking bound of image.

cc @vfdev-5 @datumbox

To resolve issue pytorch#4818
Add assert function and logic after checking bound of image
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Nov 4, 2021

💊 CI failures summary and remediations

As of commit f580b5f (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job Step Action
CircleCI binary_linux_conda_py3.9_cu111 packaging/build_conda.sh 🔁 rerun

1 job timed out:

  • binary_linux_conda_py3.9_cu111

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

Failures seems related, can you have a look?

@puhuk
Copy link
Contributor Author

puhuk commented Nov 4, 2021

Sure, let me see and send again :)
Thanks for review.

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.

Thanks for the PR. I left a couple of comments, let me know what you think.

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.

@puhuk Thanks it looks great.

Before merging could you also add tests to cover for:

  1. Corner cases such values right on the threshold
  2. Ensure that the code fails when an incorrect value is passed

@puhuk
Copy link
Contributor Author

puhuk commented Nov 4, 2021

@datumbox, sure let me add test.
But may I ask where can I add test and is there any code I can refer to?

@datumbox
Copy link
Contributor

datumbox commented Nov 4, 2021

@puhuk You can find similar tests here.

I think we need two tests:

  • One that receives an all white (255) 4x4 image and checks that the method works as expected for threshold 255.
  • One that passes the wrong threshold and checks that it fails. Here is an example of a test that checks for failures.

@puhuk
Copy link
Contributor Author

puhuk commented Nov 4, 2021

@datumbox Thanks for sharing! Let me check and send PR

@datumbox
Copy link
Contributor

@puhuk Some of your tests are failing and they are related to your changes.

@puhuk
Copy link
Contributor Author

puhuk commented Nov 15, 2021

@datumbox, I'm trying to fix this. Could you give me some time.

@datumbox
Copy link
Contributor

@puhuk Sure thing. Sometimes contributors don't give access to the CircleCi and cant see the errors, so we give a nudge to ensure they are aware. Since you know it and actively work on it, there is no rush. :)

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.

Thanks @puhuk, I think we can simplify the unit-tests a bit. Let me know what you think.

@datumbox datumbox merged commit 1c9ccb7 into pytorch:main Nov 29, 2021
@github-actions
Copy link

Hey @datumbox!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Nov 30, 2021
Summary:
* Update functional_tensor.py

To resolve issue #4818
Add assert function and logic after checking bound of image

* Update functional_tensor.py

* Update test

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Update test_functional_tensor.py

* Fix linter

Reviewed By: NicolasHug

Differential Revision: D32694295

fbshipit-source-id: c988e438a6a2ddcf272e151161956c21791690c6

Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
@puhuk puhuk deleted the issue_4818 branch May 3, 2022 07:44
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