Change interface of mcwf to match that of master

[david-pl]

I have changed the syntax of timeevolution.mcwf to accept the kwarg rates as in timeevolution.master.

It now accepts rates as a vector of floats of the same length as J. I also thought about implementing an internal diagonalization, such that if rates is a matrix, a new set of jump operators and rates is calculated using diagonljumps. However, since usually one computes many trajectories using the MCWF method, this would be expensive as the calculation is repeated for every trajectory. Instead, timeevolution.mcwf now throws an error if rates is a matrix, which points the user towards using diagonaljumps a priori.

The same changes were made to mcwf_h, but not mcwf_nh. I don't see why anyone would use mcwf_nh instead of mcwf if the rates are not already included in the Hamiltonian. Should we add this?

Please review @bastikr.

[coveralls]

Coverage increased (+0.002%) to 99.398% when pulling fd63a0f on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[coveralls]

Coverage increased (+0.002%) to 99.398% when pulling fd63a0f on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[codecov]

Codecov Report

Merging #174 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   99.39%   99.41%   +0.01%     
==========================================
  Files          32       32              
  Lines        2155     2206      +51     
==========================================
+ Hits         2142     2193      +51     
  Misses         13       13

Impacted Files	Coverage Δ
src/mcwf.jl	100% <100%> (ø)	⬆️
src/metrics.jl	100% <0%> (ø)	⬆️
src/timecorrelations.jl	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 193a427...66263c0. Read the comment docs.

[bastikr]

We should probably change the mcwf implementation to be more similar to the master implementation:

• Allow rates to be Void (if rates are already included in jump operators).
• Check all arguments (similarly to check_master).
• Only construct the non-hermitian Hamiltonian if all operators are sparse or dense.
• Specialized implementations of dmcwf_h and dmcwf_nh for rates::Void and rates::Vector.
• The jump function probably needs some additional consideration.

I agree that throwing an error with a message that tells the user about the diagonaljumps function is the right approach.

[david-pl]

I agree that we should try to stay as similar as possible to the master implementation and I am already working on it. Checking the arguments as in check_master and including the rates kwarg in the Hermitian implementation is straightforward. However, I am having some problems with the non-Hermitian version.

The non-Hermitian (nH) Hamiltonian is built up and then passed to dmcwf_nh which makes of course sense due to efficiency. Now, if we allow passing of rates::Vector to dmcwf_nh the rates have to be included in an already built up nH Hamiltonian. This can only be faithfully done if we subtract -0.5im*J*Jdagger from the nH Hamiltonian and add the jump operators again including the rates, which makes little sense.

We could simply check whether the rates are of type Void when building up the nH Hamiltonian and leave it to the user to take care of the rates when explicitly using the function mcwf_nh. Do you know of a better way to do this?

[coveralls]

Coverage decreased (-0.5%) to 98.861% when pulling 3fee639 on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[coveralls]

Coverage decreased (-0.5%) to 98.861% when pulling 3fee639 on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[bastikr]

Sorry I'm not sure if I understand your question regarding the mcwf_nh version correctly. Of course the user has to provide the correct non-Hermitian Hamiltonian corresponding to the given rates and not simply H - 0.5im*j*jdagger. Then this all should work out automatically?

[bastikr]

The latest version looks good! The only thing that I would like to change is including the rates into the jump operators (sqrt.(rates).*J). It would be better to never do this explicitly but to write different jump implementations depending on the type of the rates. This would be similar to the different dmaster methods for rates being Void, a vector or a matrix.

[coveralls]

Coverage increased (+0.01%) to 99.407% when pulling 243e1ac on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[david-pl]

In your first response you mentioned dmcwf_nh when talking about different implementations for the types of rates. This function already takes the nH Hamiltonian as argument, so it wouldn't make sense to provide different versions ofdmcwf_nh for different types of rates. That is what I meant, sorry if it wasn't clear previously.

Oh, of course I should have implemented different jump functions, my bad. I'm on it!

[coveralls]

Coverage decreased (-0.2%) to 99.184% when pulling 3716a08 on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[coveralls]

Coverage increased (+0.01%) to 99.411% when pulling 87852b8 on david-pl:mcwf-syntax into 193a427 on qojulia:master.

[coveralls]

Coverage increased (+0.01%) to 99.411% when pulling 66263c0 on david-pl:mcwf-syntax into 193a427 on qojulia:master.