Skip to content

support ForwardDiff 1.0 #2643

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

oscardssmith
Copy link
Member

No description provided.

@oscardssmith
Copy link
Member Author

ok we finally have enough to start testing this.

@pjaap
Copy link

pjaap commented Apr 9, 2025

I really looking forward to this. This currently blocks the upgrade to ForwardDiff v1 in VoronoiFVM.

@ChrisRackauckas
Copy link
Member

The biggest issue is likely with differentiation of interpolations because we want to make sure it doesn't change the discontinuity handling behavior. So there's a bit to do here.

@oscardssmith
Copy link
Member Author

I think this might be ready now. Let's see if tests agree. The only change needed was a change to the interpolation tests to use a left-derivative since [email protected] makes the derivative a right derivative by default.

@ChrisRackauckas
Copy link
Member

Is it breaking to our users that the ForwardDiff 1.0 uses the right derivative by default? We have a continuity argument, when using ForwardDiff that argument is no longer respected? Is that breaking? Or is that ForwardDiff behavior that is breaking which the user only gets if they bump to v1 as well so it's okay?

@ChrisRackauckas
Copy link
Member

Should we be correcting the forwarddiff branch in our continuity handling?

@oscardssmith
Copy link
Member Author

Is it breaking to our users that the ForwardDiff 1.0 uses the right derivative by default?

I don't think so.

We have a continuity argument, when using ForwardDiff that argument is no longer respected?

This would be breaking if our interpolations used ForwardDiff internally, but they don't, so it's not a problem.

@ChrisRackauckas
Copy link
Member

Okay so we respect ForwardDiff's branch choice and not the continuity one? I guess it's reasonable because in this context it's kind of undefined, there's two things that can choose the branch, and arguably the ForwardDiff one is more user-facing.

So okay we go with it.

@JoshuaLampert
Copy link
Contributor

Is there anything left to do or can this be merged? I think there are plenty of repos waiting for this, such that they can test ForwardDiff v1 in their repos.

@oscardssmith
Copy link
Member Author

I need to find an actual fix to test/regression/hard_dae but that is the last remaining item. The problem is in the test, but finding a version of this test that is valid for both [email protected] and ForwardDiff@1 is a little tricky

@oscardssmith
Copy link
Member Author

lets see if the rebase magically fixes the failure? I'm very confused here because this is exactly the test I was running locally and it was passing there...

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.

4 participants