-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Adapt C_ReLU, ReLU6, FlexibleReLU #3445
Conversation
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍 |
Keep open |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took so long to get to this! I think the implementation looks good, with the exception that CReLU
should implement ComputeOutputDimensions()
. I have a handful of small implementational comments that actually concern code you didn't write, but I figured was worth addressing while looking at this.
I know it's been a long time that this PR has been open, so if you don't have time anymore I can try to fix the issues by pushing directly to the branch when I'm able to.
(Also we should update HISTORY.md before we merge this.)
Thanks for working on this! And sorry again it took so long to get back to.
outputTemp.fill(6.0); | ||
output = arma::zeros<OutputType>(arma::size(input)); | ||
output = arma::zeros<MatType>(arma::size(input)); | ||
output = arma::min(arma::max(output, input), outputTemp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one could be improved too. The simple code
output = arma::min(arma::max(input, 0.0), 6.0);
should be sufficient; no need for outputTemp
.
{ | ||
const arma::colvec desiredActivations("0 3 0 6 24 \ | ||
2 0 0 0 0 0 \ | ||
100.2 0 1 0 0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like the last several elements of this can be removed. The size should be 2x the size of activationData
.
Co-authored-by: Ryan Curtin <ryan@ratml.org>
make code efficient
Thanks for handling the issues. I ran the code locally and found a few errors in my suggestions... to make things easy, I opened a PR for this branch in your fork of mlpack, so that you can review the changes before merging: AdarshSantoria#1 . Let me know if you find any issues there... I think those changes should fix all the CI problems (we'll see I guess). |
Sorry I was little busy during my sem exams and hence not able to find time for handling those issues. Thanks for solving these, I will get back to you soon. |
No worries! There's definitely no hurry, given that I let this sit for more than two months before reviewing it 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think everything is good to go here. The failing tests look unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second approval provided automatically after 24 hours. 👍
Thanks @AdarshSantoria! |
No description provided.