Skip to content
This repository has been archived by the owner on Jan 9, 2018. It is now read-only.

adds sigmoid non-linearity and tests #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mneary1
Copy link

@mneary1 mneary1 commented Aug 1, 2016

wrote a sigmoid non-linearity to test if I know what I'm doing. The tests are copied from the other non-linearities and they seem to work fine. I'm still not sure about the jacobian adjoint, and I feel strongly that I did not do it correctly.

@DickJC123
Copy link
Member

I reviewed your code and it's correct, including the jacobian adjoint, which for a pointwise operation is the same as the jacobian. Nice work!

I see your coding style matches that of tanh. I have one small objection with that coding style, namely the labeling of the jacobian method's parameter as 'dx'. When this method is used for the jacobian adjoint, the actual argument (if named explicitly) is often called 'grad' or 'dy'. Passing an argument 'dy' (even if anonymously) into the function parameter 'dx' might be confusing to some. I would write it as:

private def derivative(x: Field) = sigmoid(x) * (1f - sigmoid(x))
def jacobian(dx: Field): Field = derivative(input.forward) * dx
def jacobianAdjoint(grad: Field): Field = derivative(input.forward) * grad

override val inputs: Map[Symbol, GradientPort] = Map('input -> GradientPort(input, jacobian, jacobianAdjoint)

Don't rush to make this change though- Ben should make the call on coding standards for DifferentiableFields.

@mneary1
Copy link
Author

mneary1 commented Aug 3, 2016

Thanks for the feedback! I'll wait to see what Ben says about the style.

@bchandle
Copy link
Member

Dick is correct about preferred style. I'm happy to accept your code as-is, however, and tweak the style on Sigmoid when I fix Tanh.

The only modification necessary before merging is a license header on the files you've added. If you made these changes while working under an HPE contract, you can just copy-paste the existing language from another file. If not, you're the copyright holder and should change the copyright line to your name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants