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__ to MultiScaleRoIAlign #2840

Merged
merged 5 commits into from
Oct 21, 2020
Merged

Conversation

frgfm
Copy link
Contributor

@frgfm frgfm commented Oct 20, 2020

Hello there 👋

I happen to be playing around with region pooling architectures lately, and printing the model in the console is useful to debug or figure out ways to improve the architecture. While RoIPool and RoIAlign have useful __repr__ attributes, their multi-scale counterpart doesn't have any.

I figured this isn't worth opening an issue, so here is what this PR does:

  • adding information in the __repr__ attribute to the MultiScaleRoIAlign class
  • adding a corresponding unittest

Suggestions are welcomed for the proposed formatting!

@@ -258,3 +258,6 @@ def forward(
result = _onnx_merge_levels(levels, tracing_results)

return result

def __repr__(self) -> str:
return f"{self.__class__.__name__}(output_size={self.output_size}, sampling_ratio={self.sampling_ratio})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can also add featmap_names as it is also a user's input ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering what to do about this: I left it out, since I believe the FPN does not display it as well. But happy to reconsider, if you think it's a good addition 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added so that it's comprehensive on this __repr__ at least

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks !

if you think it's a good addition

I have no strong opinion on that. Just thought that featmap_names is a configuration provided by user and repr displays 2 of 3 its configs.

since I believe the FPN does not display it as well

Seems like there is no repr at all for FPN, https://github.com/pytorch/vision/blob/master/torchvision/ops/feature_pyramid_network.py

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.

LGTM! Thanks for the PR @frgfm !

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #2840 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2840      +/-   ##
==========================================
+ Coverage   73.35%   73.36%   +0.01%     
==========================================
  Files          99       99              
  Lines        8785     8787       +2     
  Branches     1387     1387              
==========================================
+ Hits         6444     6447       +3     
  Misses       1916     1916              
+ Partials      425      424       -1     
Impacted Files Coverage Δ
torchvision/ops/poolers.py 98.07% <100.00%> (+1.01%) ⬆️

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 99bf4c0...c4818cb. Read the comment docs.

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!

@vfdev-5 vfdev-5 merged commit e280f61 into pytorch:master Oct 21, 2020
@frgfm frgfm deleted the msroialign-repr branch October 21, 2020 09:16
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* feat: Added __repr__ to MultiScaleRoIAlign

* test: Added unittest for __repr__ of MultiScaleRoIAlign

* feat: Added feature map names in __repr__

* test: Updated unittest

Co-authored-by: vfdev <[email protected]>
vfdev-5 added a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* feat: Added __repr__ to MultiScaleRoIAlign

* test: Added unittest for __repr__ of MultiScaleRoIAlign

* feat: Added feature map names in __repr__

* test: Updated unittest

Co-authored-by: vfdev <[email protected]>
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