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

Integrate cpp package #5251

Merged
merged 8 commits into from
Mar 22, 2017
Merged

Integrate cpp package #5251

merged 8 commits into from
Mar 22, 2017

Conversation

conopt
Copy link
Contributor

@conopt conopt commented Mar 5, 2017

As mentioned in dmlc/MXNet.cpp#71, we propose to merge mxnet.cpp into mxnet project.

It will attract more attention to mxnet.cpp, and it will also enable integrated test with mxnet, so that future checkins to mxnet will not break mxnet.cpp. Meanwhile, it is important for industrial environment (e.g, data center), where python and other dependencies are normally not installed.

@piiswrong
Copy link
Contributor

I think merging is a good idea. Not sure about submodule though. We have too many submodules already

@conopt
Copy link
Contributor Author

conopt commented Mar 6, 2017

I also prefer copying mxnet.cpp directly to using submodule, as it allows direct reference to dependencies like c_api.h and dmlc-core. However, this will diverge the cpp package into two versions, one in dmlc/mxnet.cpp, and the other one in dmlc/mxnet, which is confusing. Maybe we can just move all the development to mxnet?

@piiswrong
Copy link
Contributor

Yes. We can mark mxnet.cpp as discontinued.

@hjk41
Copy link
Contributor

hjk41 commented Mar 7, 2017

We will mark dmlc/mxnet.cpp as deprecated. Future checkins will be made only in dmlc/mxnet.

@piiswrong
Copy link
Contributor

Can one of the admins verify this patch?

@conopt conopt changed the title Integrate cpp package [WIP] Integrate cpp package Mar 9, 2017
@piiswrong
Copy link
Contributor

any updates on this?

@conopt
Copy link
Contributor Author

conopt commented Mar 10, 2017

I'm updating it to remove the submodule, and to include headers from mxnet.

@conopt
Copy link
Contributor Author

conopt commented Mar 10, 2017

I have some question about BatchNorm. It recently added two arguments gamma and beta, which don't have default values. But I found that in python example codes, these arguments can be omitted. This makes mxnet.cpp unable to generate correct function signature for BatchNorm.

@conopt conopt changed the title [WIP] Integrate cpp package Integrate cpp package Mar 11, 2017
@piiswrong
Copy link
Contributor

not sure what you mean. Are you talking about initializer? what default value?

@conopt
Copy link
Contributor Author

conopt commented Mar 12, 2017

For example, the default value for eps in batchnorm is 1e-3 (https://github.com/dmlc/mxnet/blob/master/src/operator/batch_norm-inl.h#L37), and gamma and beta have no default values(https://github.com/dmlc/mxnet/blob/master/src/operator/batch_norm.cc#L81), while some example code calls batchnorm without specifying gamma and beta (https://github.com/dmlc/mxnet/blob/master/tests/python/common/models.py#L16).

@piiswrong
Copy link
Contributor

gamma and beta are weights, not parameters. their default values are set by initializer

@piiswrong
Copy link
Contributor

ndarray-or-symbol replaced NDArray and Symbol for types.

@conopt
Copy link
Contributor Author

conopt commented Mar 18, 2017

This pull request is ready for review now.

@piiswrong
Copy link
Contributor

Any idea why windows tests failed?

@conopt
Copy link
Contributor Author

conopt commented Mar 19, 2017

Fixed

@hjk41 hjk41 merged commit 21b2da0 into apache:master Mar 22, 2017
@conopt conopt deleted the cpp-package branch May 20, 2017 19:01
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.

3 participants