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

add finite_difference_jvp #191

Merged
merged 12 commits into from
Jan 31, 2025
Merged

add finite_difference_jvp #191

merged 12 commits into from
Jan 31, 2025

Conversation

oscardssmith
Copy link
Member

Add the pushforward operation with implementation taken from jacobian but simplified. This is my first time touching this code, so let me know if there's anything obviously wrong here.

@gdalle
Copy link
Member

gdalle commented Oct 3, 2024

Thank you for giving it a try! I'll try to review a bit later but I think like the other operators, this would be most useful with a Cache?

@ChrisRackauckas
Copy link
Member

Yes it needs a cache.

@gdalle
Copy link
Member

gdalle commented Nov 16, 2024

Hi @oscardssmith, friendly ping on this one, do you think you can complete it? Otherwise the performance of sparse Jacobians with DI+FiniteDiff will be bad, so I think this might be a priority before switching OrdinaryDiffEq to DI.

@oscardssmith
Copy link
Member Author

oh yeah, I can finish this off.

Add the pushforward operation with implementation taken from jacobian but simplified.
@oscardssmith oscardssmith force-pushed the os/finite_difference_jvp` branch from f51be8e to 82fc83a Compare January 27, 2025 21:50
@oscardssmith
Copy link
Member Author

ok, this is finally updated to have a VJP cache. I've given it a little testing, and will add some actual tests, but I think this is mostly ready to merge!

@gdalle
Copy link
Member

gdalle commented Jan 28, 2025

Thanks @oscardssmith! I don't see any mention of a Cache type in the PR changes though, are you sure we mean the same thing by it?

@oscardssmith
Copy link
Member Author

something went very wrong in the git process

@gdalle
Copy link
Member

gdalle commented Jan 28, 2025

image

@oscardssmith
Copy link
Member Author

now it actually contains the thing it was supposed to :)

@gdalle
Copy link
Member

gdalle commented Jan 28, 2025

This version passes my preliminary DI correctness tests! Note that I didn't benchmark performance or allocations or anything, I just used your code inside DI.pushforward(f, ::AutoFiniteDiff, ...).

For testing here, perhaps the easiest option is to augment the jacobian tests with their JVP counterparts?

@gdalle
Copy link
Member

gdalle commented Jan 28, 2025

Cross-referencing what will become the ensuing DI PR: JuliaDiff/DifferentiationInterface.jl#705

@oscardssmith
Copy link
Member Author

Tests added (and some of the forward mode tests for jacobians have been strengthened from 1e-4 to 1e-6 because they were aggressively loose tests before)

@oscardssmith
Copy link
Member Author

Tests are passing. Time to merge?

@gdalle
Copy link
Member

gdalle commented Jan 28, 2025

I can make a review pass tomorrow but I'm not familiar with this package so I'd rather have someone more confident with FiniteDiff give an opinion?

@oscardssmith
Copy link
Member Author

Looks like @ChrisRackauckas has merged all PRs in the past couple months so do you want to review?

fx1,
fdtype :: Type{T1} = Val{:forward},

Non-Allocating Cache Constructor.
Copy link
Member

Choose a reason for hiding this comment

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

well it does make a copy of x

Copy link
Member

Choose a reason for hiding this comment

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

yes the non-allocating cache is expected to just use the arrays as given 😅

@gdalle
Copy link
Member

gdalle commented Jan 29, 2025

we should probably also increment the minor version?

@gdalle
Copy link
Member

gdalle commented Jan 29, 2025

Any way to get code coverage information? Codecov protests here (and on master too):

Upload failed: {"message":"Token required because branch is protected"}

@gdalle
Copy link
Member

gdalle commented Jan 30, 2025

gentle ping, this is basically finished I think, although codecov would be nice?

@ChrisRackauckas ChrisRackauckas merged commit 6748bf8 into master Jan 31, 2025
5 checks passed
@ChrisRackauckas ChrisRackauckas deleted the os/finite_difference_jvp` branch January 31, 2025 02:52
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