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

fix a bug described in issue #488 #489

Merged
merged 4 commits into from
May 24, 2018
Merged

fix a bug described in issue #488 #489

merged 4 commits into from
May 24, 2018

Conversation

KovenYu
Copy link
Contributor

@KovenYu KovenYu commented May 6, 2018

@@ -377,6 +378,13 @@ def __init__(self, size, padding=0, pad_if_needed=False):
self.size = (int(size), int(size))
else:
self.size = size

if isinstance(padding, numbers.Number):

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@soumith
Copy link
Member

soumith commented May 17, 2018

@KovenYu hi, wonder if you had time to make the changes yet

@KovenYu
Copy link
Contributor Author

KovenYu commented May 20, 2018

@soumith Hi, that's my pleasure and I've been excited to have this chance... but I'm not familiar with Github's work flow.. shall I open a new PR, or can I directly modify in this PR? thanks

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 20, 2018

@KovenYu you can actually just commit to your fork's branch KovenYu:master and these modifications appear here too.

@KovenYu
Copy link
Contributor Author

KovenYu commented May 20, 2018

@vfdev-5 oh I got it. it works. thank you man! :)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 20, 2018

@KovenYu you're welcome! Could you also please improve the docs of RandomCrop method as we discussed earlier: possible padding values, similar to functional part ?

@KovenYu
Copy link
Contributor Author

KovenYu commented May 21, 2018

@vfdev-5 thank you! yes I'm glad to; I was just not sure whether I could do that :) would you please review it?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 21, 2018

@KovenYu looks good!

Just remarked that padding_mode has beed merged in the functional part, so maybe we can add this argument with fill ?

@KovenYu
Copy link
Contributor Author

KovenYu commented May 22, 2018

@vfdev-5 thx!

sry I'm not quite sure what you mean..

what do you mean by "padding_mode has been merged"?

do you mean there was some merging in the master branch, so I should make some adaptive changes? though I checked the master branch and seemed that my fork is aligned with it in RandomCrop and pad..

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 22, 2018

@KovenYu Sorry, maybe I explained badly what I wanted to propose.

If you take a look at source of F.pad you will see that there are supplementary options: fill and padding_mode. So, I thought that it could be good to add them to RandomCrop too, at least padding_mode...

@KovenYu
Copy link
Contributor Author

KovenYu commented May 22, 2018

@vfdev-5 got it. fairly reasonable! let's just do it and see what they say.

@fmassa
Copy link
Member

fmassa commented May 22, 2018

In addition, there are some conflicts that should be addressed before we can merge this.

@KovenYu
Copy link
Contributor Author

KovenYu commented May 22, 2018

@fmassa yeah. done.

@fmassa
Copy link
Member

fmassa commented May 22, 2018

@pytorchbot retest this please

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!

@fmassa fmassa merged commit f27ecce into pytorch:master May 24, 2018
@KovenYu
Copy link
Contributor Author

KovenYu commented May 25, 2018

my pleasure!

varunagrawal pushed a commit to varunagrawal/vision that referenced this pull request Jul 23, 2018
* fix a bug described in issue pytorch#488

* improve doc described in issue pytorch#488

* add arguments in RandomCrop as proposed by vfdev-5 in PR pytorch#489
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.

4 participants