qojulia / QuantumOptics.jl

Library for the numerical simulation of closed as well as open quantum systems.
http://qojulia.org
Other
518 stars 101 forks source link

Add const checking for non-dynamic evolution #371

Closed amilsted closed 11 months ago

Krastanov commented 11 months ago

May we make the error message a bit more detailed? I am imagining an unexperienced undergrad that has not read the documentation very thoroughly. Maybe something along the lines of:

"You are attempting to use a time-dependent dynamics generator (a Hamiltonian or Lindbladian) with a solver that assumes constant dynamics. To avoid errors, please use the _dynamic solvers instead, e.g. schroedinger_dynamic instead of schroedinger."

Longer term, I am wondering why is there an error at all if we already know what should actually happen. Should we just dispatch to the correct solver if we detect ! is_const (with maybe a @warn warning)? No need to fix this here, but it can maybe be posted on the issue tracker and marked as "good first issue"?

Otherwise, looks good to me (presumably after the is_const fixes get merged to QOBase.jl).

amilsted commented 11 months ago

Yeah, I agree we should probably just "do the right thing" longer term and not require the "dynamic" methods. We could just add appropriate set_time!() to the solvers everywhere, which would compile out for common const cases. Or just branch on is_const, as you suggest.

codecov[bot] commented 11 months ago

Codecov Report

Merging #371 (17b4aad) into master (3dacb02) will decrease coverage by 0.02%. The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
- Coverage   97.82%   97.80%   -0.02%     
==========================================
  Files          18       18              
  Lines        1516     1550      +34     
==========================================
+ Hits         1483     1516      +33     
- Misses         33       34       +1     
Files Changed Coverage Δ
src/stochastic_base.jl 97.22% <75.00%> (-2.78%) :arrow_down:
src/bloch_redfield_master.jl 92.68% <100.00%> (+0.18%) :arrow_up:
src/master.jl 100.00% <100.00%> (ø)
src/mcwf.jl 100.00% <100.00%> (ø)
src/schroedinger.jl 94.11% <100.00%> (+0.11%) :arrow_up:
src/stochastic_master.jl 93.90% <100.00%> (+0.23%) :arrow_up:
src/stochastic_schroedinger.jl 100.00% <100.00%> (ø)
src/timeevolution_base.jl 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

amilsted commented 11 months ago

Should be ready now @Krastanov @ChristophHotter

Krastanov commented 11 months ago

LGTM. I assume the repeated definition of _check_const is just to avoid mixing namespaces/modules? I am fine with it if it is on purpose.

amilsted commented 11 months ago

Yeah, that's the only reason.