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

Update scalar.jl to use muladd #102

Merged
merged 5 commits into from
Jan 9, 2023
Merged

Update scalar.jl to use muladd #102

merged 5 commits into from
Jan 9, 2023

Conversation

ptiede
Copy link
Contributor

@ptiede ptiede commented Dec 22, 2022

This updates scalar.jl to use muladd generically for ScaledShiftedLogistic since JuliaDiff/DiffRules.jl#28 has been merged. I also used muladd instead of fma since it only uses fma if it does give a speed boost. Additionally, I was having issues using Enzyme.jl with fma inserted while everything seems to work fine with muladd.

@ptiede
Copy link
Contributor Author

ptiede commented Dec 22, 2022

The errors seems to be from ADgradient not being defined. Is that some versioning issue with LogDensityProblems?

@tpapp
Copy link
Owner

tpapp commented Dec 22, 2022

ptiede#1 should fix CI.

@tpapp tpapp closed this Dec 22, 2022
@tpapp tpapp reopened this Dec 23, 2022
@tpapp
Copy link
Owner

tpapp commented Dec 23, 2022

Sorry, it was closed accidentally.

Copy link
Owner

@tpapp tpapp left a comment

Choose a reason for hiding this comment

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

Can you please add a test for what was fixed?

Eg a simple logdensity that has a transformation involving ScaledShiftedExp, AD with Enzyme, check that logdensity_and_gradient is finite.

Copy link
Owner

@tpapp tpapp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tpapp tpapp merged commit 3f55cc8 into tpapp:master Jan 9, 2023
@ptiede
Copy link
Contributor Author

ptiede commented Jan 9, 2023

Sorry for the delay! Holidays happened, and I got stuck with a recent Enzyme bug that just got fixed. Did you want me to add an additional test in another PR?

@tpapp
Copy link
Owner

tpapp commented Jan 9, 2023

It is fine as it is, but of course tests are always welcome if you have time. Thanks again for the PR.

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.

2 participants