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

Adapt C_ReLU, ReLU6, FlexibleReLU #3445

Merged
merged 26 commits into from Jun 12, 2023
Merged

Conversation

AdarshSantoria
Copy link
Contributor

No description provided.

@mlpack-bot
Copy link

mlpack-bot bot commented Apr 16, 2023

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! 👍

@mlpack-bot mlpack-bot bot added the s: stale label Apr 16, 2023
@AdarshSantoria
Copy link
Contributor Author

Keep open

@mlpack-bot mlpack-bot bot closed this Apr 27, 2023
@rcurtin rcurtin reopened this May 29, 2023
Copy link
Member

@rcurtin rcurtin left a 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.

src/mlpack/methods/ann/layer/c_relu_impl.hpp Outdated Show resolved Hide resolved
src/mlpack/methods/ann/layer/c_relu.hpp Show resolved Hide resolved
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);
Copy link
Member

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.

src/mlpack/methods/ann/layer/relu6_impl.hpp Show resolved Hide resolved
{
const arma::colvec desiredActivations("0 3 0 6 24 \
2 0 0 0 0 0 \
100.2 0 1 0 0");
Copy link
Member

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.

@rcurtin
Copy link
Member

rcurtin commented May 30, 2023

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).

@AdarshSantoria
Copy link
Contributor Author

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.

@rcurtin
Copy link
Member

rcurtin commented May 30, 2023

No worries! There's definitely no hurry, given that I let this sit for more than two months before reviewing it 😄

Copy link
Member

@rcurtin rcurtin left a 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.

@mlpack-bot mlpack-bot bot removed the s: stale label Jun 9, 2023
Copy link

@mlpack-bot mlpack-bot bot left a 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. 👍

HISTORY.md Outdated Show resolved Hide resolved
@rcurtin rcurtin merged commit c387028 into mlpack:master Jun 12, 2023
0 of 5 checks passed
@rcurtin
Copy link
Member

rcurtin commented Jun 12, 2023

Thanks @AdarshSantoria!

This was referenced Jun 14, 2023
@AdarshSantoria AdarshSantoria deleted the notadapted branch July 1, 2023 06:42
@IWNMWE IWNMWE mentioned this pull request Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants