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

Exposing LevelMapper params in MultiScaleRoIAlign (pytorch#3129) #3151

Merged
merged 5 commits into from
Jan 5, 2021

Conversation

baldassarreFe
Copy link
Contributor

Ability to set the canonical_scale and canonical_level of LevelMapper from the init method of MultiScaleRoIAlign.
Discussion in: #3129

Before merging:

  • should additional tests be written?
  • please verify docstring style and links, I am not sure about the correct syntax.

Ability to set the `canonical_scale` and `canonical_level` of LevelMapper from the init method of MultiScaleRoIAlign. 
Not sure if this is the right place to expose those parameters. 
Also, additional tests should be written.

Discussion in issue pytorch#3129
@facebook-github-bot
Copy link

Hi @baldassarreFe!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #3151 (2c45d19) into master (83171d6) will decrease coverage by 0.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3151      +/-   ##
==========================================
- Coverage   73.14%   72.76%   -0.38%     
==========================================
  Files          99       99              
  Lines        9256     8981     -275     
  Branches     1493     1431      -62     
==========================================
- Hits         6770     6535     -235     
+ Misses       2015     1999      -16     
+ Partials      471      447      -24     
Impacted Files Coverage Δ
torchvision/ops/poolers.py 86.79% <100.00%> (+0.25%) ⬆️
torchvision/ops/_register_onnx_ops.py 48.78% <0.00%> (-4.35%) ⬇️
torchvision/transforms/transforms.py 82.07% <0.00%> (-1.62%) ⬇️
torchvision/transforms/functional.py 80.83% <0.00%> (-0.69%) ⬇️
torchvision/ops/__init__.py 100.00% <0.00%> (ø)
torchvision/transforms/__init__.py 100.00% <0.00%> (ø)
torchvision/transforms/autoaugment.py
torchvision/ops/new_empty_tensor.py 80.00% <0.00%> (ø)
torchvision/transforms/functional_pil.py 68.37% <0.00%> (+0.76%) ⬆️
torchvision/transforms/functional_tensor.py 73.49% <0.00%> (+1.02%) ⬆️

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 90645cc...0bfa526. 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.

LGTM, thanks a lot!

Adding more tests would be good, but they can be added in a follow-up PR (as this PR only exposes functionality which was already there).

@fmassa fmassa merged commit 6315358 into pytorch:master Jan 5, 2021
facebook-github-bot pushed a commit that referenced this pull request Jan 21, 2021
Summary:
* Exposing LevelMapper params in MultiScaleRoIAlign

Ability to set the `canonical_scale` and `canonical_level` of LevelMapper from the init method of MultiScaleRoIAlign.
Not sure if this is the right place to expose those parameters.
Also, additional tests should be written.

Discussion in issue #3129

* MultiScaleRoIAlign keyword-only parameters + docs (#3129)

* Linter fix (trailing whitespace)

Reviewed By: datumbox

Differential Revision: D25954558

fbshipit-source-id: 4c54b17925fed520e38b35d8b76f8148499ea943

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

3 participants