-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
add ClipValue and ClipNorm #1133
Conversation
Keras has these, good to have it on Flux as well |
@bhvieira The docstrings are simpified now. |
Please add them to the tests as well |
Should both of these be called clipping, given that one clamps (only components outside range) and the other scales (all components)? Also they act on the gradient not the value. Maybe Also: is the norm of the whole array always what you want? If this is some batch of observations, then it may make more sense to scale each of them by its own norm. In which case this would need to take |
norm uses scalar getindex. This throws an error when working with CuArrays. eg:
The issue comes from CuArrays. However, |
I think it's better to just follow Keras's convention. Gradients are not batched, so... |
@HenriDeh Thanks for pointing that out. |
Oh sorry, this only applies to parameter updates, not to gradients in general. Which is obvious in the code, |
Since this PR is just a simple feature add-on, how about merging this? |
bors try |
tryBuild succeeded: |
We need usage examples in the docstrings and we should add the two to the |
this seems good to go, thanks bors r+ |
Build succeeded: |
No description provided.