return_problem keyword added to QuantumOptics integrators

[henhen724]

This feature allows you to make any QuantumOptics solver return the SciML problem rather than trying to solve the problem immediately. This allows end users to more easily incorporate QuantumOptics with other SciML packages and features.

Here is a simple example of how to use it:

using QuantumOptics
using OrdinaryDiffEq

basis = SpinBasis(1 // 2)
su = spinup(basis)
u0 = dense(identityoperator(basis))
sx = sigmax(basis)
sz = sigmaz(basis)

# for the time dependent equation
f(t, psi) = sx * π
tspan = 0:1.0
t, u = timeevolution.schroedinger(tspan, u0, π * sx)

rslt = timeevolution.schroedinger(tspan, u0, π * sx, return_problem=true)
sol = OrdinaryDiffEq.solve(rslt["prob"], rslt["alg"]; rslt["solve_kwargs"]...)
tout = rslt["out"].t
psi_t = rslt["out"].saveval

Sorry about all formatting changes my VSCode did that automatically.

[Krastanov]

Hi, Henry! Thanks for the contribution! A few minor stylistic things to get out of the way before a real review:

• do not put Manifest.toml in the pull request -- this is a machine generated file that just lists the exact version of each package you have installed
• Having a dictionary of string keys is not particularly natural in Julia. In Python or JavaScript this might be necessary, but julia has separate types for "string as in a container sequence for arbitrary characters" and for "string as in a name to refer to something, e.g. a configuration option". For the later you would use symbols in julia. https://stackoverflow.com/questions/23480722/what-is-a-symbol-in-julia
• In idiomatic julia we have a function do one thing, without completely changing its meaning depending on the value of a keyword argument. I think it would be more idiomatic to have schroedinger_problem instead of schroedinger(...; return_problem=true). That way it would also be easier to simplify schroedinger itself.

This pull request also seems to be related to the following issue: #298

[Krastanov]

I know that this is frustrating to hear, but I would also be reluctant to merge this with all of the formatting changes. They might be for the better, and it kinda makes sense to run JuliaFormatter and enforce it in future pull requests, but that should be done in a separate single-purpose pull request, otherwise reviewing would be too difficult and using git blame will become increasingly cumbersome.

[codecov]

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.61%. Comparing base (d2c71fe) to head (dcc8234).
Report is 12 commits behind head on master.

Files	Patch %	Lines
src/mcwf.jl	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
- Coverage   97.03%   96.61%   -0.43%     
==========================================
  Files          18       19       +1     
  Lines        1554     1686     +132     
==========================================
+ Hits         1508     1629     +121     
- Misses         46       57      +11     

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

[henhen724]

Ok, I will go back and incorporate all your comments. Remove the toml, get rid of the formating changes, switch the dict to using string keys to one using symbols, and fix the two lines missing in the code coverage.

The one thing that concerns me is the "I think it would be more idiomatic to have schroedinger_problem instead of schroedinger(...; return_problem=true)." I am fine with putting in the work to do that, but I just want to make sure you realize that it would mean redefining all of the integrators in QuantumOptics now call their equivalent _problem function. That will mean a lot of edits. Is that okay?

[Krastanov]

To your last question, I do suspect I am missing something. My original thinking was that making something like schroedinger_problem would actually involve less code and fewer if-else statements than the setup you have built. Maybe it would be worthwhile to discuss this first on just one of the solvers before doing it to all of them.

Another minor point is that NamedTuple probably makes more sense here than a Dict, as we will not be storing arbitrary keys (kinda related to the first comment of string vs symbol). But I also hope switching to a schroedinger_problem setup would obviate that need altogether.

[henhen724]

Currently, I only need to modify the functions integrate, integrate_stoch, and integrate_mcwf, because the functions that call them already pass all kwargs through to them. If I do what you are proposing, I will need to change the body of stochastic.schroedinger, stochastic.schroedinger_dynamic, stochastic.master, stochastic.master_dynamic, etc. I agree that it is in the end a cleaner way of programing it, but is just going to be a lot of edits. Again, I am willing to do it, but wanted to discuss it before hand to make sure it's not going to go to waist.

As for the NameTuple suggestion, noted. I had not used that before. I think you still want the NamedTuple in this case because you want to know what QuantumOptics' default solver arguments would have been. It's useful if you're trying to compair different runs. Also makes writing the tests simipler.

[henhen724]

I can't get the formatting to match here may be a tab vs spaces thing

[Krastanov]

the diff tool gets confused occasionally -- this probably will be resolved when we squash

[Krastanov]

Thanks for proceeding with the changes! Below I try to answer the questions you have raised and I also try to give a few git tips that might make your work easier (you might already know some or all of them, but I am posting just in case).

Named tuple containing configuration options or solver kwargs makes sense. In particular, it would be nice if it can be structured in a way where the named tuple (or an entry in that tuple) can be directly destructured. E.g. for a function f(;a=..., b=...) you can do namedtup = (;a=..., b=...) and then f(;namedtup...) automatically populates the kwargs.

I would suggest trying it out just for one of the solvers for now just to make sure we are both happy with the approach and amount of work involved.

After you explained which functions you are currently changing, I noticed another issue: the current version of this pull request is modifying private functions. On its own, that is fine, but if the point is to make this generally available to all users, these functions need to be made part of the public API -- however they were never meant to be user facing, rather only an internal implementation detail (that we probably want to remove when the broadcasing interface improvements by @apkille land). E.g. currently we have this:

Concerning git workflow:

I noticed you were undoing some of the changes in a somewhat manual fashion. Of course, you should do it in whatever way is most convenient for you, but you might be interested in the following options:

• git cherrypick and git checkout can bring into your current work folder a file or change from another branch/commit
• the -i flag for many git commands provides you with an interactive way to add only certain files or parts of files, if you do not want to commit all of the changes you have made in a given file
• similarly, the vs code IDE has a convenient graphical interface for staging/committing only fractions of a file
• git commit --amend can modify the last commit
• git rebase -i provides you with an interactive rebase terminal interface that can be used to reorder or squash commits
• generally do not hesitate to close PRs and open related PRs if that is an easier way to undo a change

[apkille]

@henhen724 #404 and qojulia/QuantumOpticsBase.jl#172 are also good PRs to look at, they contain a lot of examples. You can create ODEProblems with QuantumOptics types already, but there's a performance regression (it's not terrible but noticeable when you benchmark on larger problems). SciML uses its own FastBroadcast.jl package to deal with broadcasting arrays faster, while we use the standard Julia broadcasting interface on our QO types. I'm currently working on reducing the performance overhead.

[Krastanov]

Hi there! I will mark this as a draft, just to make it easier to manage my review queue. Please do not hesitate to ping us if there is anything we can help with on this PR.