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

Making autodiff Jacobian reusable #211

Merged
merged 8 commits into from
Jul 21, 2024

Conversation

JonasKoziorek
Copy link
Contributor

I have separated the ForwardDiff.jl jacobian from the tangent_rule. This way we can reuse this in PeriodicOrbits.jl.

If you find everything ok, I will modify the docstrings.

@JonasKoziorek
Copy link
Contributor Author

I am unsure about the Jacobianconfiguration. Is this ok?

function _jacobian(ds, ::Val{true})
f = dynamic_rule(ds)
u0 = current_state(ds)
cfg = ForwardDiff.JacobianConfig(
(du, u) -> f(du, u, p, p), deepcopy(u0), deepcopy(u0)
)
Jf = (J0, u, p, t) -> begin
uv = @view u[:, 1]
du = copy(u)
ForwardDiff.jacobian!(
J0, (du, u) -> f(du, u, p, t), view(du, :, 1), uv, cfg, Val{false}()
)
nothing
end
return Jf
end

Especially line 17. I suppose that du is there just as a container.

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

yeah this seems fine to me. In the future don't wait for a review to add the documentation. The documentation is the most important part and is what you should be writing first anyways!

@JonasKoziorek
Copy link
Contributor Author

Please review if it position of the jacobian.jl file and the docs position is ok.

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

Perfect. Don't forget to increment minor version in Project.toml and add a changelog entry.

@Datseris
Copy link
Member

ah, also tests fail. Can you check that please?

@JonasKoziorek
Copy link
Contributor Author

The test fails for successful_step of ParallelDynamicalSystem. It is not related to the changed in this PR. I will try to fix it.

@Datseris
Copy link
Member

If you want to fix it, thanks, as it is not related to your GSOC, but please do so in another PR!

@Datseris Datseris merged commit 89fcd8b into JuliaDynamics:main Jul 21, 2024
1 of 2 checks passed
@JonasKoziorek JonasKoziorek deleted the jacobian branch July 21, 2024 14:30
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