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

Normalize supports arbitrary dimensions #767

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fmassa
Copy link
Contributor

@fmassa fmassa commented Apr 13, 2016

Adds docs + tests.
Also, cleans a bit some unnecessary temporary variables.

Important remark: I added the dimension parameter as the second argument to the constructor, making the eps the third one. This breaks code that changed the default eps during initialization, but I felt that dim coming after eps felt weird. Let me know if you prefer to let dim the last argument.

@apaszke
Copy link
Contributor

apaszke commented Apr 13, 2016

@fmassa I think if you're changing the order of arguments like here, it's worth adding a check that dim is an integer. This would at least throw an error if somebody used the eps argument now.

@fmassa
Copy link
Contributor Author

fmassa commented Apr 13, 2016

@apaszke I agree. just pushed a diff checking that dim is integer.

@fmassa
Copy link
Contributor Author

fmassa commented Apr 13, 2016

I forgot to consider that previously saved models do not have the dimfield. I'm pushing a patch

@fmassa
Copy link
Contributor Author

fmassa commented Apr 13, 2016

Fixed case of old saved models. To make it simple I just added the default value in the forward/backward calls, I didn't overwrite the read method.

Assert that dim is integer in Normalize
@fmassa
Copy link
Contributor Author

fmassa commented Apr 19, 2016

One note about the negative indexes, if you want I could also add a numInputDims parameter, as in nn.Max. Let me know what you think.

@soumith soumith closed this Jun 6, 2016
@soumith soumith reopened this Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants