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.

Sign up for GitHub

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

Jump to bottom

Use the latest ReverseDiff #97

Merged
merged 4 commits into from
Jul 29, 2020
Merged

Use the latest ReverseDiff #97

merged 4 commits into from
Jul 29, 2020

Conversation

mohamed82008
Copy link
Member

The latest version of ReverseDiff (JuliaRegistries/General#18125) has a lot of the functionality in DistributionsAD. This PR removes the code already ported to RD and sets a lower bound on RD's version. Tests should pass after the RD PR is merged.

@mohamed82008 mohamed82008 reopened this Jul 18, 2020
@mohamed82008 mohamed82008 mentioned this pull request Jul 18, 2020
Closed
@mohamed82008
Copy link
Member Author

There is an issue with the latest ReverseDiff version. I am on it.

@mohamed82008
Copy link
Member Author

CC: @ChrisRackauckas

@ChrisRackauckas
Copy link

I'm also seeing something a bit odd: https://travis-ci.org/github/SciML/DiffEqFlux.jl/jobs/709405023

@mohamed82008
Copy link
Member Author

Almost fixed it locally.

@mohamed82008
Copy link
Member Author

Once tests pass locally, I will push the fixes.

@ChrisRackauckas
Copy link

No worries, thanks for being on it. Out of curiosity, what aspect does it seem to be pointing to?

@mohamed82008
Copy link
Member Author

It was a name clash mostly. I defined _add_to_deriv! for the grad macro and another for broadcasting. Wrong methods were called. Also some functions were defined in propagation.jl and used in broadcast.jl but propagation.jl was included after broadcast.jl!

@mohamed82008 mohamed82008 mentioned this pull request Jul 18, 2020
Merged
@mohamed82008 mohamed82008 reopened this Jul 18, 2020
@mohamed82008
Copy link
Member Author

The remaining failures are ambiguity errors when constructing Wishart and InverseWishart probably caused by the a change in Distribution.jl.

@mohamed82008
Copy link
Member Author

I fixed all the errors locally. Once JuliaStats/Distributions.jl#1148 gets merged and released, I will push the fixes. If the Distributions.jl maintainers don't like the PR, I will workaround it by defining TuringMatrixBeta but let's wait and see first.

@yebai
Copy link
Member

yebai commented Jul 20, 2020

We should rebase the changes in #95 before merging this PR.

@mohamed82008
Copy link
Member Author

Fair enough.

@mohamed82008 mohamed82008 mentioned this pull request Jul 28, 2020
Closed
@nmheim nmheim mentioned this pull request Jul 28, 2020
Open
devmotion
devmotion reviewed Jul 28, 2020
View reviewed changes
src/matrixvariate.jl Outdated Show resolved Hide resolved
devmotion
devmotion reviewed Jul 28, 2020
View reviewed changes
src/matrixvariate.jl Outdated Show resolved Hide resolved
devmotion
devmotion reviewed Jul 28, 2020
View reviewed changes
src/matrixvariate.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

I fixed some merge conflicts arising from recent changes in the master branch. Hopefully I didn't mess up anything 🤞

@devmotion
Copy link
Member

Anything else that you want to include in this PR @mohamed82008?

@mohamed82008
Copy link
Member Author

LGTM, thanks @devmotion!

@mohamed82008 mohamed82008 merged commit 4eacdf7 into master Jul 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the mt/use_latest_reversediff branch July 29, 2020 17:22
@yebai yebai mentioned this pull request Jul 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devmotion devmotion devmotion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mohamed82008 @ChrisRackauckas @yebai @devmotion