-
Notifications
You must be signed in to change notification settings - Fork 7k
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
add typehints for torchvision.datasets.phototour #2531
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2531 +/- ##
=======================================
Coverage 70.68% 70.69%
=======================================
Files 94 94
Lines 8017 8018 +1
Branches 1276 1276
=======================================
+ Hits 5667 5668 +1
Misses 1945 1945
Partials 405 405
Continue to review full report at Codecov.
|
There was a problem hiding this 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 think we should be careful with any renamings that we do in our internal variables which are accessible to the users. In this case, the changes you did can be considered as BC-breaking.
For the sake of this PR, can you revert the renaming?
'notredame_harris': 0.4854, 'yosemite_harris': 0.4844, 'liberty_harris': 0.4437} | ||
std = {'notredame': 0.1864, 'yosemite': 0.1818, 'liberty': 0.2019, | ||
'notredame_harris': 0.1864, 'yosemite_harris': 0.1818, 'liberty_harris': 0.2019} | ||
means = {'notredame': 0.4854, 'yosemite': 0.4844, 'liberty': 0.4437, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in theory a BC-breaking change.
I think it would be better to avoid renaming this in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it BC breaking? I mean after the assignment (L75-76) the dicts are no longer accessible. This only matters if one uses the dicts from the class directly PhotoTour.mean
or PhotoTour.std
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just saw this, yeah, this was most probably an oversight in the original implementation, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reply, LGTM!
'notredame_harris': 0.4854, 'yosemite_harris': 0.4844, 'liberty_harris': 0.4437} | ||
std = {'notredame': 0.1864, 'yosemite': 0.1818, 'liberty': 0.2019, | ||
'notredame_harris': 0.1864, 'yosemite_harris': 0.1818, 'liberty_harris': 0.2019} | ||
means = {'notredame': 0.4854, 'yosemite': 0.4844, 'liberty': 0.4437, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just saw this, yeah, this was most probably an oversight in the original implementation, good catch!
No description provided.