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.

Sign up for GitHub

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

Jump to bottom

Changes to assert to ValueError in achor utils #2960

Merged
merged 3 commits into from
Nov 6, 2020
Merged

Changes to assert to ValueError in achor utils #2960

merged 3 commits into from
Nov 6, 2020

Conversation

oke-aditya
Copy link
Contributor

Fixes #2947

Just a small change to ValueError instead of assert. This will enhance debugging.

fmassa
fmassa requested changes Nov 4, 2020
View reviewed changes
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 for the PR!

I believe there is an issue with the logic, can you double-check?

torchvision/models/detection/anchor_utils.py Outdated Show resolved Hide resolved
torchvision/models/detection/anchor_utils.py Outdated
Comment on lines 105 to 106
raise ValueError('''Achors should be Tuple[Tuple[int]] because each feature
map could potentially have different sizes and aspect ratios ''')
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a single ", and not break the line as currently done, as it introduces artifacts on the print?
Also, can you make the error message a bit more descriptive, as you can have Tuple[Tuple[int]] being passed but still have the same problem. The underlying issue is that there needs to be a match between the number of feature maps passed and the number of sizes / aspect ratios specified.

@oke-aditya
Copy link
Contributor Author

Extremely sorry for such rookie error in code 🙏
I have fixed it 👍 Also improved the error message.

@oke-aditya oke-aditya requested a review from fmassa November 4, 2020 19:07
@codecov
Copy link

codecov bot commented Nov 4, 2020 •
edited
Loading

Codecov Report

Merging #2960 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2960      +/-   ##
==========================================
- Coverage   73.43%   73.41%   -0.02%     
==========================================
  Files          99       99              
  Lines        8813     8814       +1     
  Branches     1391     1392       +1     
==========================================
- Hits         6472     6471       -1     
- Misses       1916     1917       +1     
- Partials      425      426       +1
Impacted Files Coverage Δ
torchvision/models/detection/anchor_utils.py 89.61% <0.00%> (-2.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5c0bfa...3c7f0bb. Read the comment docs.

fmassa
fmassa approved these changes Nov 6, 2020
View reviewed changes
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 a lot!

For a follow-up PR, do you think you could send a PR adding a test for this ValueError?
Maybe in a file called test_models_detection_anchor_utils.py?

@fmassa fmassa merged commit 46f6083 into pytorch:master Nov 6, 2020
@oke-aditya
Copy link
Contributor Author

Yep I will

@oke-aditya oke-aditya deleted the fix_assert branch November 6, 2020 11:28
@oke-aditya oke-aditya mentioned this pull request Nov 7, 2020
Merged
@fmassa fmassa mentioned this pull request Nov 16, 2020
Closed
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
@oke-aditya @bryant1410
Changes to assert to ValueError in achor utils (pytorch#2960)
7511b71 
* changes to value error

* fixes it :)

* simpler fix
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
@oke-aditya @vfdev-5
Changes to assert to ValueError in achor utils (pytorch#2960)
58b32ff 
* changes to value error

* fixes it :)

* simpler fix
@oke-aditya oke-aditya mentioned this pull request Jan 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmassa fmassa fmassa approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace assert with ValueError in AnchorGenerator
3 participants
@oke-aditya @fmassa @facebook-github-bot