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

Added __repr__ attribute to GeneralizedRCNNTransform #1834

Merged
merged 8 commits into from
Feb 4, 2020

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Jan 29, 2020

This PR aims at:

  • adding __repr__ attribute to the GeneralizedRCNNTransform class

Suggestions are welcomed for the proposed formatting!

See #1786

frgfm added 3 commits January 29, 2020 19:14
Added more details to default __repr__ attribute for printing.
Switched strings with  syntax to f-strings.
@frgfm frgfm requested a review from fmassa January 29, 2020 18:29
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!

Can you also add some basic tests, to ensure that the printing gives correct results?
They can go in test_models.py for example

Checked integrity of __repr__ attribute
@codecov-io
Copy link

codecov-io commented Feb 1, 2020

Codecov Report

Merging #1834 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1834      +/-   ##
=========================================
- Coverage    0.43%   0.43%   -0.01%     
=========================================
  Files          92      92              
  Lines        7388    7396       +8     
  Branches     1112    1113       +1     
=========================================
  Hits           32      32              
- Misses       7348    7356       +8     
  Partials        8       8
Impacted Files Coverage Δ
torchvision/models/detection/transform.py 0% <0%> (ø) ⬆️
torchvision/models/inception.py 0% <0%> (ø) ⬆️

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 a791479...dd05796. Read the comment docs.

Fixed the formatted strings in the __repr__ integrity check for GeneralizedRCNNTransform
@frgfm frgfm requested a review from fmassa February 1, 2020 21:08
@fmassa
Copy link
Member

fmassa commented Feb 3, 2020

Thanks for the test!

Tests for Python 3.5 are failing, probably because f-strings have been introduced in Python 3.6. Can you fix this?

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.

See my comment, tests are failing due to f-strings

frgfm added 3 commits February 3, 2020 17:23
Switched back f-strings to .format syntax for Python3.5 compatibility.
Fixed multiple-line string syntax for compatibility
Fixed formatting of min_size argument of the resizing part
@frgfm frgfm requested a review from fmassa February 3, 2020 17:50
@frgfm
Copy link
Contributor Author

frgfm commented Feb 3, 2020

@fmassa I have no clue why the coverage of inception has changed, but all the other tests are passing!

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!

The coverage for inception is probably a false positive.

@fmassa fmassa merged commit e2573a7 into pytorch:master Feb 4, 2020
@frgfm frgfm deleted the transform-repr branch February 4, 2020 10:31
fmassa pushed a commit to fmassa/vision-1 that referenced this pull request Jun 9, 2020
* feat: Added __repr__ attribute to GeneralizedRCNNTransform

Added more details to default __repr__ attribute for printing.

* fix: Put back relative imports

* style: Fixed pep8 compliance

Switched strings with  syntax to f-strings.

* test: Added test for GeneralizedRCNNTransform __repr__

Checked integrity of __repr__ attribute

* test: Fixed unittest for __repr__

Fixed the formatted strings in the __repr__ integrity check for GeneralizedRCNNTransform

* fix: Fixed f-strings for earlier python versions

Switched back f-strings to .format syntax for Python3.5 compatibility.

* fix: Fixed multi-line string

Fixed multiple-line string syntax for compatibility

* fix: Fixed GeneralizedRCNNTransform unittest

Fixed formatting of min_size argument of the resizing part
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants