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

Caffe Plugin#2746

Merged
piiswrong merged 67 commits intoapache:masterfrom
Taco-W:master
Jul 27, 2016
Merged

Caffe Plugin#2746
piiswrong merged 67 commits intoapache:masterfrom
Taco-W:master

Conversation

@piiswrong
Copy link
Contributor

@piiswrong piiswrong commented Jul 17, 2016

@HrWangChengdu I helped you created this pull request to track progress and make some comments.

Taco-W added 30 commits May 23, 2016 09:58
@Taco-W
Copy link
Contributor

Taco-W commented Jul 25, 2016

@piiswrong could you please check python side code, i.e. example/caffe/caffe_net.py?
I guess
fc = mx.sym.CaffeOp(data=[data0, data1, data2], prototxt=....)
seems better than
fc = mx.sym.CaffeOp(data_0=data0, data_1=data1, data_2=data2, prototxt=....)

DMLC_DECLARE_FIELD(in_num).set_range(0, 100).set_default(1)
DMLC_DECLARE_FIELD(data_num).set_range(0, 100).set_default(1)
.describe("Operator input number");
DMLC_DECLARE_FIELD(w_num).set_range(0, 100).set_default(0)
Copy link
Contributor Author

@piiswrong piiswrong Jul 25, 2016

Choose a reason for hiding this comment

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

change to num_weight

@piiswrong
Copy link
Contributor Author

piiswrong commented Jul 25, 2016

data=[data0, data1, data2] is equivalent to *[data0, data1, data2].
Also please change the names. MXNet convention is usually num_xxx instead of xxx_num.

Other than that its good.
Thanks a lot

PS. also please rebase to master

@piiswrong
Copy link
Contributor Author

LGTM. Fix lint and we can merge.

This is a great effort. Thanks so much for doing this.

@Taco-W
Copy link
Contributor

Taco-W commented Jul 27, 2016

Fixed the lint and complement the patch in caffe docs.
Is there anything else to do?
Thank you so much for helping me doing this merge!

@piiswrong
Copy link
Contributor Author

piiswrong commented Jul 27, 2016

Hopefully the test will pass and I'll merge it

You can add more features in new PRs

@piiswrong piiswrong merged commit e343416 into apache:master Jul 27, 2016
@piiswrong
Copy link
Contributor Author

@HrWangChengdu Because I created this pull request for you, the commit history now traces this to me. You can make a new PR with two commits where the first one removes every file you added in this PR and the second one adds them back in, so that the history will be traced to you.

@mli
Copy link
Contributor

mli commented Jul 27, 2016

you can revert this pr...

@piiswrong
Copy link
Contributor Author

How? I probably don't have the privilige

@piiswrong
Copy link
Contributor Author

@mli I can't do forced push. Can you? Discard the last commit?

@mli
Copy link
Contributor

mli commented Jul 27, 2016

forced push is disabled, which is too dangers. there is a revert button in
the pr page, it basically files another pr to revert all changes

On Tue, Jul 26, 2016 at 10:17 PM, Eric Junyuan Xie <notifications@github.com

wrote:

@mli https://github.com/mli I can't do forced push. Can you? Discard
the last commit?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2746 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAZv4XQbHpvsAxZG0vkydV4mKqoXJLSKks5qZun3gaJpZM4JOVRn
.

@antinucleon
Copy link
Contributor

I have created a revert PR

@Taco-W
Copy link
Contributor

Taco-W commented Jul 27, 2016

Should I new a PR after the revert one is done?

@piiswrong
Copy link
Contributor Author

piiswrong commented Jul 27, 2016

Yes, Please PR again

@Taco-W
Copy link
Contributor

Taco-W commented Jul 27, 2016

Just create one #2854

@tqchen
Copy link
Member

tqchen commented Jul 27, 2016

It is great that this is merged in. Let us work on making the blogpost to dmlc.ml and put this on the news

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants