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

Fix and add more AD tests #625

Merged
merged 3 commits into from
Dec 14, 2018
Merged

Fix and add more AD tests #625

merged 3 commits into from
Dec 14, 2018

Conversation

mohamed82008
Copy link
Member

This PR extends #624 and adds another workaround for binomlogpdf and ForwardDiff. It also adds tests for these AD functions and fixes other broken AD tests.

REQUIRE Outdated
@@ -11,6 +11,7 @@ MacroTools
StatsFuns 0.7.0
SpecialFunctions
Bijectors
FDM
Copy link
Member

Choose a reason for hiding this comment

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

Move the dependency on FDM to test/REQUIRE?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that's what I thought I did! Thanks for pointing this out.

@yebai
Copy link
Member

yebai commented Dec 13, 2018

Thanks, @mohamed82008. This PR looks good to me. Just one minor comment about dependency, see code review.

Copy link
Member

@trappmartin trappmartin left a comment

Choose a reason for hiding this comment

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

Looks good!

x = map(_->Float64(_), vi[nothing])
∇E = gradient_reverse(x, vi, ad_test_f)
x = map(x->Float64(x), vi[nothing])
∇E = gradient_reverse(x, vi, ad_test_f)[2]
Copy link
Member

Choose a reason for hiding this comment

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

Is the [2] on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the first value seems to be negative the logp, whereas the second is the gradient. This is defined here.

@yebai yebai merged commit 873aaec into master Dec 14, 2018
@mohamed82008 mohamed82008 deleted the mt/ad_tests branch February 9, 2019 09:40
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