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

params(false) does indeed create a new/copy #7977

Closed
danielschonfeld opened this issue Jul 5, 2019 · 1 comment
Closed

params(false) does indeed create a new/copy #7977

danielschonfeld opened this issue Jul 5, 2019 · 1 comment
Labels
Bug Bugs and problems DL4J General DeepLearning4j issues

Comments

@danielschonfeld
Copy link

Issue Description

Documentation error/typo

Version Information

Latest

Please indicate relevant versions, including, if relevant:

DL4J

Additional Information

params(boolean) method states that if backwardsOnly=false, you do NOT get a copy, and changes to the given variable will change the underlying network. The code is opposite of that, in fact if you pass false, you get a fresh copy unrelated to the pointer inside the original network it was created in.

Contributing

I'll be happy to submit a PR if you'll tell me what you'd like done

@AlexDBlack
Copy link
Contributor

This impacts both MultiLayerNetwork and ComputationGraph.
The javadoc does indeed not match the behaviour.

Thinking about this, I'm thinking we should simply remove that params(boolean) method entirely. I don't think it's particularly useful at this point. The no-args params() method is useful, and the paramTable() method is also useful. For most nets, params() and params(boolean) will be the same. For nets with autoencoder, VAE etc layers, the params(boolean) method is supposed to cut out the decoder weights/bias - but there's almost nothing you can actually use that vector for in practice (maybe transfer learning, but it's safer and less error prone to use paramTable() I think).

@AlexDBlack AlexDBlack added Bug Bugs and problems DL4J General DeepLearning4j issues labels Jul 5, 2019
AlexDBlack added a commit that referenced this issue Jul 20, 2019
* #7977 deprecate legacy MultiLayerNetwork/ComputationGraph.params(boolean) method

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* Fix bad test

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* Small fixes

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* Fix Histogram mapping

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* Fix incorrect name handling in DifferentialFunction

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* More fixes

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* Histogram fixes

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* Proper histogram fix

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* ToString/NDArrayStrings fix

Signed-off-by: AlexDBlack <blacka101@gmail.com>

* JSON UTF8 serialization fix

Signed-off-by: AlexDBlack <blacka101@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs and problems DL4J General DeepLearning4j issues
Projects
None yet
Development

No branches or pull requests

2 participants