Skip to content

Adding soft shrink function#2174

Merged
birm merged 80 commits intomlpack:masterfrom
ojhalakshya:adding-soft-shrink-function
Mar 13, 2020
Merged

Adding soft shrink function#2174
birm merged 80 commits intomlpack:masterfrom
ojhalakshya:adding-soft-shrink-function

Conversation

@ojhalakshya
Copy link
Copy Markdown
Contributor

I have added the soft shrink activation function.
It can be referred from link.
I have also tried to do the same according to the mlpack standards, any other improvements are appreciated and would be happy to work more.

@mlpack-bot
Copy link
Copy Markdown

mlpack-bot bot commented Feb 3, 2020

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Tests that are added follow the testing guide
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

@kartikdutt18
Copy link
Copy Markdown
Member

Hi @ojhalakshya, could you rebase your repo, It will fix python error that might occur (It's unrelated to your PR).

@ojhalakshya ojhalakshya force-pushed the adding-soft-shrink-function branch from 0e5347e to 70fd586 Compare March 12, 2020 06:51
@rcurtin
Copy link
Copy Markdown
Member

rcurtin commented Mar 12, 2020

This code touches the ANN code, and there was recently a big refactoring of the ANN code in #2259, so be sure to merge the master branch into your branch here to make sure that nothing will fail if this PR is merged. 👍 (I'm pasting this message into all possibly relevant PRs.)

@ojhalakshya
Copy link
Copy Markdown
Contributor Author

Hey @rcurtin @zoq
Can you have a look at this pr when you get time, it is ready from my side I guess.
Have already fixed the changes asked earlier.
Thanks.

Copy link
Copy Markdown
Member

@kartikdutt18 kartikdutt18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ojhalakshya, for working on this, just one minor comment. Thanks.

Comment on lines +59 to +61
* noise level sigma of the input(noisy image) and a
* coefficient 'a' which is one of the training parameters.
* Default value of lambda is 0.5.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ojhalakshya, could you please fix the indentation here. Thanks.

* \end{array}
* \right.
* @f}
* lambda is set to 0.5 by default.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can put this with the constructor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @favre49
I am afraid I have included this there also, line no 61 - 63.
Also these are important here because here I have defined the formula.
Thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I said this is because this seemed like an implementation detail that would belong with the constructor, not with the overview of the class, and putting it in both places would be redundant

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @favre49
I have implemented the changes.
Kindly see the same if they are satisfactory.
Thanks.

@ojhalakshya ojhalakshya requested review from birm, favre49 and zoq March 13, 2020 08:28
@ojhalakshya ojhalakshya force-pushed the adding-soft-shrink-function branch from 8b87115 to 2858662 Compare March 13, 2020 10:33
Copy link
Copy Markdown
Member

@favre49 favre49 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything is alright. Thanks for the good work!

Copy link
Copy Markdown
Member

@birm birm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, and adapting to #2259

@birm birm merged commit 2196966 into mlpack:master Mar 13, 2020
@ojhalakshya
Copy link
Copy Markdown
Contributor Author

I am very happy to be of help!
Thanks.

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.

9 participants