Add ExponentialUtilities propagator#97
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #97 +/- ##
========================================
+ Coverage 90.2% 90.4% +0.2%
========================================
Files 35 37 +2
Lines 2558 2614 +56
========================================
+ Hits 2307 2361 +54
- Misses 251 253 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi! The main logic is in Note: I checked the expv path: the operator stays sparse when convert_operator is set appropriately. For sparse Liouvillians, a big speedup comes from passing e.g. |
|
Codecov patch coverage is below target but checks are green The remaining misses look like extension boilerplate. Happy to add more tests if you prefer. |
There was a problem hiding this comment.
Thank you for your PR! I really appreciate the effort for adding an propagator based on ExponentialUtilities
I feel like this needs to be pulled apart a little bit into different PRs
-
Identify how the interface requirements for ExponentialUtilities interact with QuantumPropagators. Revise
QuantumPropagators.Interfacesaccordingly. I think this probably only affectscheck_operator. Make sure everything inQuantumPropagatorsimplements the extended interface -
If applicable, make a PR to QuantumGradientGenerators to implement and test any changes to the interface requirements in (1). The goal would be to make sure the gradient-vectors and gradient-generators will work fine with ExponentialUtilities.
-
Implement the propagator with ExponentialUtilities (this PR), taking into a ccount the comments from the code review. Add tests for GRAPE-relevant propagation, but not a GRAPE optimization as test.
-
(Followup) Add a test to GRAPE that uses the new propagation (adapted from the existing tests in this PR, which should be moved to GRAPE)
-
(Followup) Consider performance enhancements:
- Keeping a pre-allocated
ScaledOperatorfor the-𝕚 H - Can we use the non-allocating
expv!? - Maybe it would be good to take a stab at SciML/ExponentialUtilities.jl#176, which would get around most of these issues.
- Keeping a pre-allocated
| struct _SizedOp{A} | ||
| A::A | ||
| end | ||
|
|
||
| Base.size(op::_SizedOp) = size(op.A) | ||
| Base.size(op::_SizedOp, dim::Integer) = size(op.A)[dim] | ||
| Base.eltype(op::_SizedOp) = eltype(op.A) | ||
| LinearAlgebra.ishermitian(op::_SizedOp) = LinearAlgebra.ishermitian(op.A) | ||
| LinearAlgebra.mul!(y, op::_SizedOp, x) = mul!(y, op.A, x) | ||
|
|
||
| _ensure_size_dim(A) = | ||
| if hasmethod(size, Tuple{typeof(A),Int}) | ||
| A | ||
| else | ||
| (hasmethod(size, Tuple{typeof(A)}) ? _SizedOp(A) : A) | ||
| end |
There was a problem hiding this comment.
This doesn't seem like something that should exist.
The problem here is just size being a required interface for ExponentialUtilities. That should be solved by adding a size to any type that's missing it, and adding a check for size to the interface requirements / test routines
There was a problem hiding this comment.
By the way, eltype must be defined for a type, not an instance of that type
There was a problem hiding this comment.
I've added a commit to remove these
There was a problem hiding this comment.
Sorry about that, this was leftover code from an attempt to get the :gradgen path working with GRAPE. After narrowing the scope to only support gradgen_method=:taylor I should have removed this but missed it.
| if nameof(typeof(propagator.state)) == :GradVector && | ||
| nameof(parentmodule(typeof(propagator.state))) == :QuantumGradientGenerators | ||
| throw( | ||
| ArgumentError( | ||
| "ExponentialUtilities propagation does not support GRAPE `gradient_method=:gradgen`. " * | ||
| "Use `gradient_method=:taylor` instead." | ||
| ) | ||
| ) | ||
| end |
There was a problem hiding this comment.
Remove this check.
What do we need to do to make gradient generators work with this propagator?
More generally, the assumptions that go into any component of a propagator need to be checked by the check_* routines in QuantumPropagators.Interfaces.
| if propagator.backward | ||
| dt = -dt | ||
| end | ||
| dt_expv = complex(dt) |
There was a problem hiding this comment.
ExponentialUtilities.expv doesn't support a real-valued t? That seems odd.
| state, | ||
| generator, | ||
| tlist; | ||
| method=ExponentialUtilities, # or :expv |
There was a problem hiding this comment.
Remove # or :expv. Passing a Module is the only supported interface
| backward=false, | ||
| verbose=false, | ||
| parameters=nothing, | ||
| expv_kwargs=(; ishermitian=false), # set for non-Hermitian generators |
There was a problem hiding this comment.
We do define LinearAlgebra.ishermitian for operators, and we could require it as part of the formal Generator/Operator interface. So that means we should probably not pass it manually as part of expv_kwrags by default.
Seems to me the default expv_kwargs should be empty.
On top of that, there's something I'm pretty confused about here: If the generator is Hermitian, then the ScaledOperator(-1im, H) that you're using is not Hermitian. So how does that work?
| Base.similar(::GradVector, ::Type{T}, dims::Tuple{Int,Int}) where {T} = | ||
| Matrix{T}(undef, dims...) | ||
| Base.similar(::GradVector, ::Type{T}, dims::Tuple{Int}) where {T} = | ||
| Vector{T}(undef, dims[1]) |
There was a problem hiding this comment.
Why are these methods necessary?
If they are, we should add a comment explaining their purpose.
On top of that, should similar return the same type (a GradVector)?
I haven't decided on whether a state must always be one-dimensional, but converting into a Matrix seems very odd at this point.
There was a problem hiding this comment.
They are part of the AbstractArray interface, and ExpinentialUtilities indeed relies on them. I’ve taken steps on master to remedy this, and this PR has been rebased on that. Some rough edges around these interface questions may still remain
|
|
||
| Base.size(O::Operator) = size(O.ops[1]) | ||
| Base.size(O::Operator, dim::Integer) = size(O.ops[1], dim) | ||
| Base.eltype(O::Operator) = eltype(O.ops[1]) |
There was a problem hiding this comment.
Only looking at ops[1] doesn't seem safe
| include("test_grape_exputils_liouvillian.jl") | ||
| end | ||
|
|
||
| println("\n* Time-dependent observables (test_timedependent_observables.jl):") |
There was a problem hiding this comment.
I'd be happy to have tests like this as part of the GRAPE package, but for the test suite of QuantumPropagators, this should be broken up, and test the relevant components required for a GRAPE optimization.
That would definitely include forward and backward propagation of a gradient-generator
| const GHz = 1.0 | ||
| const MHz = 0.001 * GHz |
|
|
||
| eps = [ | ||
| 0.2 * | ||
| (1 + 0.05 * rand()) * |
There was a problem hiding this comment.
Make sure each use of random numbers in tests is seeded
|
I've merged #98 as a prerequisite, and rebased this PR on top of that |
|
I took a stab at addressing the issues I raised in the review. One thing I found while playing around with this In the commit I added
This needs further changes in Base.similar(::GradVector, ::Type{T}, dims::Tuple{Int,Int}) where {T} =
Matrix{T}(undef, dims...)
Base.similar(::GradVector, ::Type{T}, dims::Tuple{Int}) where {T} =
Vector{T}(undef, dims[1])
function Base.eltype(::Type{GradVector{num_controls,T}}) where {num_controls,T}
return eltype(T)
endThe This needs more thought about the implications for gradient-generators. |
|
Ugh, I missed your earlier comment
when I removed the conversion machinery. I'll have to think about that some more... |
goerz
changed the title
|
It really seems like ExponentialUtilities implicitly expects the operators and states to have the I've done this in #100. The
@krugx Can you elaborate on "the operator stays sparse when convert_operator is set appropriately". That doesn't seem like it should be necessary: if the inputs are sparse, they should remain sparse inside In relation to this, as part of this PR, we should also add a set of "baseline" tests on |
|
Thank you very much for your thorough review and the guidance, I'm sorry for the late reply. I will pull the commits to my local branch and work on it gradually over the next week or so. Just as a heads up, I'm still quite new to open source and navigating the packages of the JuliaQuantumControl organization is challenging, so it might take me a few days to get through the dev setup and follow up on PRs. I really appreciate you taking the time to help me get this merged! |
|
I've rebased this on the latest I'll see if I can add the ["baseline" tests] for |
|
@goerz Regarding your question about densification: The densification came from the default For generator/operator inputs, I'm sorry for the confusion, this densification behavior was introduced by my own bad commit, and I should have caught it earlier. There was no reason at all for this behavior at all, and you are correct, sparse inputs should remain sparse inside of This behavior is present in commit This was removed later by your commit |
|
|
||
| This is a [`PWCPropagator`](@ref). | ||
| """ | ||
| mutable struct ExpvPropagator{GT,OT,ST,KST,CT} <: PWCPropagator |
There was a problem hiding this comment.
Let's call this ExponentialUtilitiesPropagator and define it in src/exponential_utilities_propagator.jl, not in the extension module. The reason is that extension module are essentially hidden. They should only add methods, but not define new functions or types. A user of QuantumPropagators might want to do: using QuantumPropagators: ExponentialUtilitiesPropagator; propagator = init_prop(#=...=#); @assert propagator isa ExponentialUtilitiesPropagator. They can't do that if ExponentialUtilitiesPropagator is local to the extension module.
The set_t! function should probably also be in src/exponential_utilities_propagator.jl, since it doesn't use anything from ExponentialUtilities. The file [ext/QuantumPropagatorsExponentialUtilitiesExt.jl should only contain the method definition that actually rely on ExponentialUtilities such as init_prop and prop_step!. Put a comment in src/exponential_utilities_propagator.jl to point to the extension module.
There was a problem hiding this comment.
Pull request overview
Adds an ExponentialUtilities-backed piecewise-constant propagator (Krylov expv) to QuantumPropagators via a Julia package extension, and introduces accompanying docs + tests for both the underlying ExponentialUtilities behavior and end-to-end propagation.
Changes:
- Add
ExponentialUtilitiesPropagatortype insrc/and implementinit_prop/prop_step!in a newExponentialUtilitiesextension. - Add new test coverage for ExponentialUtilities’
expv/expv!behavior and QuantumPropagators propagation withmethod=ExponentialUtilities. - Extend docs to describe
method=ExponentialUtilitiesand ensure docs build loads the extension.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/exponential_utilities_propagator.jl |
Introduces the new propagator struct/type used by the extension implementation. |
ext/QuantumPropagatorsExponentialUtilitiesExt.jl |
Implements init_prop and prop_step! for the ExponentialUtilities method. |
src/QuantumPropagators.jl |
Includes the new propagator type definition in the main module. |
src/generators.jl |
Tweaks ScaledOperator plain-text display dispatch and adds an Operator sizing comment. |
Project.toml |
Adds ExponentialUtilities as a weak dependency and registers the extension + compat. |
docs/src/methods.md |
Documents the new ExponentialUtilities propagation method. |
docs/make.jl / docs/Project.toml |
Ensures ExponentialUtilities is available/loaded during doc builds and cross-linking. |
test/test_propagate.jl |
Adds propagation checks comparing ExponentialUtilities propagation to known results. |
test/test_prop_interfaces.jl |
Adds interface checks for the new propagator type. |
test/test_exputils.jl |
Adds higher-level propagation tests (Hermitian, non-inplace, Liouvillian, time-dependent generator). |
test/test_exponential_utilities.jl |
Adds broad tests for ExponentialUtilities’ expv/expv! and related Krylov workflows. |
test/runtests.jl |
Registers the two new ExponentialUtilities-related test files in the suite. |
test/Project.toml |
Adds ExponentialUtilities to the test environment dependencies. |
CLAUDE.md |
Adds documented test-writing guidance around StableRNG usage and seed generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Revised PR according to review: * Deleted `ext/QuantumPropagatorsQuantumGradientGeneratorsExt.jl` (type-piracy) * Removed convert_state/convert_operator fields and all conversion machinery * Removed :gradgen guard that rejected GradVector states * Removed :expv symbol alias * Removed ScaledOperator wrapping — instead passes -1im * dt as the time argument to ExponentialUtilities.expv. This avoids the use of `ScaledOperator` * Removed both "GRAPE note" from docs * Add ExponentialUtilities to InterLinks., use @extref link to ExponentialUtilities.expv * Add `test/test_exputils.jl` — new component-level test file replacing GRAPE integration tests
This is to explore the functionalities of the package and the guarantee that everythink works reliably.
Will moved to GRAPE.jl later for testing ExponentialUtilites
If A0 was accidentally hermitian, it caused an InexactError in the arnoldi method.
Define ExponentialUtilitiesPropagator and set_t! in src so the type is publicly accessible from QuantumPropagators. Keep the extension limited to ExponentialUtilities dependent methods.
- Document expv_kwargs parameters (m, ishermitian, tol, opnorm, iop, mode) with accurate forwarding description (arnoldi!/expv!, not just expv) - Expand methods.md with Arnoldi/Lanczos details, AbstractArray interface requirement, and supports_vector/matrix_interface trait references - Disambiguate test set names to avoid ambiguous failure reports - Convert expv return type before setfield! in non-inplace path to handle element type promotion (e.g. real state + complex time) - Add test verifying expv type promotion behavior - Add early ArgumentError for mode=:error_estimate with non-Hermitian generators instead of deferring to obscure ExponentialUtilities error - Fix struct indentation to match project conventions (4-space) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
goerz
left a comment
goerz
left a comment
There was a problem hiding this comment.
Alright, I think this is ready for merging now.
Sorry of the delay!
Summary
gradient_method=:taylor):gradgenwith:expv:gradgen