Skip to content

Conversation

@kartikdutt18
Copy link
Member

@kartikdutt18 kartikdutt18 commented Mar 13, 2020

closes #2287.
I have made changes to remove r-value from remaining functions. There were also some style changes that I thought should be made so I have also made them. I hope that's okay. I can also open another PR for style but since they were simple enough I've made them in this PR.
Thanks.

@kartikdutt18 kartikdutt18 changed the title Removed r-value references and some style fixes Removed r-value references from remaining functions [and some style fixes] Mar 13, 2020
@kartikdutt18
Copy link
Member Author

I think the build is fine, no relevant test has failed that is related to this PR.

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.

Whew, the build failures are not looking good today. Interestingly the fixes you have here actually did not cause a build failure, because HardShrink and LogCoshLoss were never actually tested as part of an FFN.

Anyway, I looked through all the build output and all failures are either:

  • Julia bindings failures (addressed by #2283)
  • OS X load/save failures (addressed by #2278 and #2271)

So it seems good to merge to me. 👍 Thanks for the fixes!

@kartikdutt18
Copy link
Member Author

Yeah, I think they were merged when your PR was already open so I think they weren't changed. Also I went through the repo twice to ensure that there are no other functions / layers that need to be refactored so I guess we have completely removed r-values references in ann (for arma datatypes).

@rcurtin
Copy link
Member

rcurtin commented Mar 13, 2020

Thanks! I think you might be more on top of things than I am. 😄

@kartikdutt18
Copy link
Member Author

Thanks a lot! Happy to help in anyway I can.

Copy link
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.

Thanks for being so quick on this :)

@favre49 favre49 merged commit 2fb1c2b into mlpack:master Mar 13, 2020
@kartikdutt18
Copy link
Member Author

Thanks a lot!

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.

Hard Shrink Activation Function does not comply to new method signatures

3 participants