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 const checking for non-dynamic evolution #371

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

amilsted
Copy link
Collaborator

No description provided.

@Krastanov
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link

codecov bot commented Jul 29, 2023

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%) ⬇️
src/bloch_redfield_master.jl 92.68% <100.00%> (+0.18%) ⬆️
src/master.jl 100.00% <100.00%> (ø)
src/mcwf.jl 100.00% <100.00%> (ø)
src/schroedinger.jl 94.11% <100.00%> (+0.11%) ⬆️
src/stochastic_master.jl 93.90% <100.00%> (+0.23%) ⬆️
src/stochastic_schroedinger.jl 100.00% <100.00%> (ø)
src/timeevolution_base.jl 100.00% <100.00%> (ø)

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

@amilsted
Copy link
Collaborator Author

Should be ready now @Krastanov @ChristophHotter

@Krastanov
Copy link
Collaborator

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
Copy link
Collaborator Author

amilsted commented Aug 1, 2023

Yeah, that's the only reason.

@amilsted amilsted merged commit 9c025b5 into qojulia:master Aug 1, 2023
5 of 7 checks passed
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