Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

Sample python bilinear initializer at integral points in y-direction#12983

Merged
sandeep-krishnamurthy merged 2 commits intoapache:masterfrom
vladoovtcharov:patch/bilinear_init
Jan 26, 2019
Merged

Sample python bilinear initializer at integral points in y-direction#12983
sandeep-krishnamurthy merged 2 commits intoapache:masterfrom
vladoovtcharov:patch/bilinear_init

Conversation

@vladoovtcharov
Copy link
Copy Markdown
Contributor

@vladoovtcharov vladoovtcharov commented Oct 26, 2018

Bilinear initializer was sampling at fractional points in y rather then sampling at integral points (like is done for x)

@vladoovtcharov vladoovtcharov requested a review from szha as a code owner October 26, 2018 16:48
@ankkhedia
Copy link
Copy Markdown
Contributor

ankkhedia commented Oct 29, 2018

@vladoovtcharov Thanks for your contribution!

@mxnet-label-bot [pr-awaiting-review, Python]

@vandanavk @Roshrini @sandeep-krishnamurthy Could you please help review the PR

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review Python labels Oct 29, 2018
anirudhacharya
anirudhacharya reviewed Oct 29, 2018
View reviewed changes
Comment thread python/mxnet/initializer.py
for i in range(np.prod(shape)):
x = i % shape[3]
y = (i / shape[3]) % shape[2]
y = (i // shape[3]) % shape[2]
Copy link
Copy Markdown
Member

@anirudhacharya anirudhacharya Oct 29, 2018

Choose a reason for hiding this comment

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

there is a _init_bilinear method in the base Initializer class - https://github.com/apache/incubator-mxnet/blob/master/python/mxnet/initializer.py#L212 . Is this class redundant?

Also I see that many of the initializers do not have tests. Could you please add a test for this change by creating a new file test_initializer.py in the tests folder. Tests for other initializers can be added to the same file later.

@ankkhedia
Copy link
Copy Markdown
Contributor

ankkhedia commented Oct 30, 2018

@sandeep-krishnamurthy Could you please label this issue as pr-awaiting-response

@sandeep-krishnamurthy sandeep-krishnamurthy added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-review PR is waiting for code review labels Oct 31, 2018
@kalyc
Copy link
Copy Markdown
Contributor

kalyc commented Nov 12, 2018

Hi @vladoovtcharov thanks for your contribution! Could you please address the comments above?

@stu1130
Copy link
Copy Markdown
Contributor

stu1130 commented Nov 21, 2018

@vladoovtcharov ping again.
let us know if you need any help!

@vladoovtcharov
Copy link
Copy Markdown
Contributor Author

vladoovtcharov commented Nov 21, 2018 •
edited
Loading

_init_bilinear looks redundant and also seems to be deprecated from the comments but has the same issue so I changed that one as well.
Some initializers have tests in incubator-mxnet/tests/python/unittest/test_init.py but not bilinear I added a quick test for now

@anirudhacharya
Copy link
Copy Markdown
Member

anirudhacharya commented Nov 21, 2018

The issue of code redundancy still remains. I am not sure about the history about this bit of code, pinging @szha to maybe help shed some light here.

@vandanavk
Copy link
Copy Markdown
Contributor

vandanavk commented Nov 29, 2018

what are the next steps on this PR @vladoovtcharov @anirudhacharya @szha ?

roywei
roywei approved these changes Dec 11, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@roywei roywei left a comment

Choose a reason for hiding this comment

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

LGTM! @vladoovtcharov thanks for the contribution, could you trigger CI by rebase or push an empty commit?

@vladoovtcharov vladoovtcharov force-pushed the patch/bilinear_init branch 3 times, most recently from 0fa8ca0 to 5e59c11 Compare December 18, 2018 05:01
@vladoovtcharov
Copy link
Copy Markdown
Contributor Author

vladoovtcharov commented Dec 18, 2018

python2 test seems to be failing (but works for python 3). I don't currently have python 2 installed but I'll try to test out soon

@vladoovtcharov
Copy link
Copy Markdown
Contributor Author

vladoovtcharov commented Dec 19, 2018

Wasn't handling division correctly in python 2. Seems to be working now

@Roshrini
Copy link
Copy Markdown
Member

Roshrini commented Jan 2, 2019

@sandeep-krishnamurthy Can you please review/merge this PR?
@mxnet-label-bot Update [Python, pr-awaiting-merge]

@marcoabreu marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Jan 2, 2019
@anirudhacharya
Copy link
Copy Markdown
Member

anirudhacharya commented Jan 14, 2019

@Roshrini can you please merge this

sandeep-krishnamurthy
sandeep-krishnamurthy approved these changes Jan 26, 2019
View reviewed changes
Copy link
Copy Markdown

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

Thanks

@sandeep-krishnamurthy sandeep-krishnamurthy merged commit 28c20fb into apache:master Jan 26, 2019
@vandanavk vandanavk mentioned this pull request Jan 31, 2019
Merged
5 tasks
stephenrawls pushed a commit to stephenrawls/incubator-mxnet that referenced this pull request Feb 16, 2019
@vladoovtcharov @stephenrawls
Sample python bilinear initializer at integral points in y-direction (a…
2807102 
…pache#12983)

* Sample python bilinear initializer at integral points in y-direction

* Add unit test for bilinear initializer
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@vladoovtcharov
Sample python bilinear initializer at integral points in y-direction (a…
88ac320 
…pache#12983)

* Sample python bilinear initializer at integral points in y-direction

* Add unit test for bilinear initializer
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@szha szha Awaiting requested review from szha szha is a code owner

4 more reviewers

@anirudhacharya anirudhacharya anirudhacharya left review comments

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy approved these changes

@Roshrini Roshrini Roshrini approved these changes

@roywei roywei roywei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-merge Review and CI is complete. Ready to Merge Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@vladoovtcharov @ankkhedia @kalyc @stu1130 @anirudhacharya @vandanavk @Roshrini @sandeep-krishnamurthy @roywei @marcoabreu