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

Implement ForwardDiff in master solvers and general DiffEq problems on QO types #409

Merged
merged 6 commits into from
Sep 14, 2024

Conversation

apkille
Copy link
Contributor

@apkille apkille commented Aug 9, 2024

using OrdinaryDiffEq, ForwardDiff, QuantumOptics

b = SpinBasis(10//1)
psi0 = spindown(b)
H(p) = p[1]*sigmax(b) + p[2]*sigmam(b)
f!(dpsi, psi, p, t) = timeevolution.dschroedinger!(dpsi, H(p), psi)
function cost(p)
    prob = ODEProblem(f!, psi0, (0.0, pi), p)
    sol = solve(prob, DP5(); save_everystep=false)
    return 1 - norm(sol[end])
end

julia> ForwardDiff.gradient(cost, [10.0, -3.0])
2-element Vector{Float64}:
 -151.2972676200132
  -67.02418550025618

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.70%. Comparing base (d2c71fe) to head (8f8c7fc).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
- Coverage   97.03%   96.70%   -0.34%     
==========================================
  Files          18       19       +1     
  Lines        1554     1701     +147     
==========================================
+ Hits         1508     1645     +137     
- Misses         46       56      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Project.toml Outdated Show resolved Hide resolved
@apkille
Copy link
Contributor Author

apkille commented Aug 11, 2024

@Krastanov Our current ForwardDiff tests are a bit finicky because we are testing numerical equality between ForwardDiff and some implementation of finite difference methods. Should we just increase the abstol or would that be too fragile? Or maybe there's a better way to test ForwardDiff here? Maybe we just use @test_nowarn?

@Krastanov
Copy link
Collaborator

increasing the tolerance should be fine (not quite sure whether it is the abs or rel one though)

@apkille
Copy link
Contributor Author

apkille commented Aug 14, 2024

@Krastanov is this good to merge?

@Krastanov Krastanov self-requested a review August 15, 2024 13:21
@Krastanov
Copy link
Collaborator

The tests currently only test whether operators and their linear algebra works, right? There are no new tests that check the actual solvers? Should we be closing #357 (I guess what tests we need here depends on the answer to that question)?

Do schroedinger, master, and the others work as well?

Could you add what you have in the first comment (the PR description) to the tests as well?

@apkille
Copy link
Contributor Author

apkille commented Aug 15, 2024

The tests currently only test whether operators and their linear algebra works, right? There are no new tests that check the actual solvers?

@Krastanov the tests do check the master solvers (timeevolution.master, timeevolution.master_h and timeevolution.master_nh), that's what the new testset is doing.

@apkille
Copy link
Contributor Author

apkille commented Aug 15, 2024

Should we be closing #357 (I guess what tests we need here depends on the answer to that question)?

Do schroedinger, master, and the others work as well?

The ForwardDiff.Dual only propagates through schroedinger, schroedinger_dynamic, and master (see #378 as well). I was going to implement ForwardDiff for master_dynamic and master_nh_dynamic in this PR, but it's slightly involved because of how the dynamic solvers currently work with input functions that create a tuple of operators, so I'm saving it for another submission. For instance, master_dynamic is written as

timeevolution.master_dynamic(tspan, rho0, f; <keyword arguments>)

where f(t, rho) -> (H, J, Jdagger) or f(t, rho) -> (H, J, Jdagger, rates). So it's tricky propagating the duals here. Same with semiclassical solvers. It's straightforward if we just define the differential equation problem from the start 😃, we have only to write a few lines of code in the internals.

@apkille
Copy link
Contributor Author

apkille commented Aug 15, 2024

Could you add what you have in the first comment (the PR description) to the tests as well?

Yes, I can do that.

@Krastanov Krastanov merged commit 2939a6a into qojulia:master Sep 14, 2024
6 of 7 checks passed
@Krastanov
Copy link
Collaborator

Wonderful work! Thank you for contributing it!

@apkille apkille deleted the forward branch September 15, 2024 16:08
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