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

Make distances autodiffable #72

Closed
wants to merge 2 commits into from
Closed

Conversation

kskyten
Copy link

@kskyten kskyten commented Jul 12, 2017

Basically I just changed the AbstractFloat types to Real for ForwardDiff and ReverseDiff compatibility. The tests still pass. Is there some reason to use the AbstractFloat type?

I just did the following quick check for AD functionality.

using Distances
import ForwardDiff
import ReverseDiff

X = rand(3, 10);
f(w) = sum(pairwise(WeightedSqEuclidean(w), X))
df(w) = ForwardDiff.gradient(f, w)
df2(w) = ReverseDiff.gradient(f, w)

df([1., 2., 3.])
df2([1., 2., 3.])

How do I verify that the AD is working properly? ReverseDiff documentation says that mutation of the input is not allowed. There are some updating assignments in the code, but as far as I know these are not mutations in Julia. Is it safe to assume that if I get results, they are correct (i.e. I wont get wrong results silently)?

@KristofferC
Copy link
Member

Could put a test dependency on forwarddiff, and use it to test that AD works. Then use either analytic or numeric differentiation to test for correctness.

@ararslan
Copy link
Member

Bump! Still to do here:

  • Fix conflicts (in particular this will require switching parametric syntaxes)
  • Add correctness tests

Once that's done we'll be good to go. Thanks for the contribution!

@KristofferC KristofferC mentioned this pull request Aug 1, 2017
@KristofferC
Copy link
Member

Should be fixed in #74

@KristofferC KristofferC closed this Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants