search

dmlc / minpy

NumPy interface with mixed backend execution
https://minpy.readthedocs.io/en/latest/
Other
1.11k stars 112 forks source link

Fix "Reshape already existed" #161

Closed Taco-W closed 7 years ago

Taco-W commented 7 years ago

Remove explicit reshapre registeration

lryta commented 7 years ago

Don't fix this. Assume they are using engine branch of MXNet as in the doc.

On Mar 16, 2017, at 1:57 PM, Haoran Wang notifications@github.com wrote:

Remove explicit reshapre registeration

You can view, comment on, or merge this pull request online at:

https://github.com/dmlc/minpy/pull/161

Commit Summary

Remove explicit reshapre registeration File Changes

M minpy/array_variants/mxnet/mxnet_core.py (2) Patch Links:

https://github.com/dmlc/minpy/pull/161.patch https://github.com/dmlc/minpy/pull/161.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

Taco-W commented 7 years ago

Would engine branch include the reshape thing?

lryta commented 7 years ago

MXNet's dev version changes op, so that reshape is now registered in the universal way (we used to hack this spot). I think we will keep this PR but merge together with the update of engine branch?

jermainewang commented 7 years ago

Where is NDArray.reshape right now?

Taco-W commented 7 years ago

It's in the latest master branch

jermainewang commented 7 years ago

Wanna to push this. I found the reshape op is still here: https://github.com/dmlc/mxnet/blob/master/python/mxnet/ndarray.py#L455 . Dont know why it does not work right now.

lryta commented 7 years ago

I found reshape in mx.nd.__dict__. That explains the conflict.

lryta commented 7 years ago

@jermainewang Just need to simply remove that line.

jermainewang commented 7 years ago

I see. Can you update the fix. Just delete it. Dont comment it since it will fail the lint check.

jermainewang commented 7 years ago

Hmm... For backward compatibility, a better fix is to check whether reshape has already been imported. If not, add the primitive as normal.

lryta commented 7 years ago

@HrWangChengdu I have no access to your repo. Can you fix the problem according to @jermainewang 's suggestion?

Taco-W commented 7 years ago

@lryta @jermainewang please check

jermainewang commented 7 years ago

Can you test it under the latest mxnet master? The travis cannot check this.

Taco-W commented 7 years ago

@jermainewang @lryta how about this?

Taco-W commented 7 years ago

I leave the PrimitiveRegistryError there in case we want to use it later

jermainewang commented 7 years ago

LGTM