Code and tutorial for multi-time correlations

[rmadw]

Hi Piper and Gerald,

As discussed in issue #105, here is a pull request for the multi-time correlations code, the modifications to the original two-time correlations code and the tutorial. I have implemented all the feedback given by Piper (thank you again!).

Going through the contribution checklist, I think all the relevant boxes are ticked now:

• [x] The contribution has been discussed and agreed on in the Issue section.
• [x] Code contributions do its best to follow the zen of python.
• [x] The automated test are all positive:
  • [x] tox -e py36 (to run pytest) the code tests.
  • [x] tox -e style (to run pylint) the code style tests.
  • [x] tox -e docs (to run sphinx) generate the documentation.
• [x] Added test for changed/added code.
• [x] API code contributions include input checks (defensive code).
• [x] API code contributions include helpful error messages. (For the two above points I followed the style of the original compute_correlations code; hope that's sufficient.)
• [x] The documentation has been updated:
  • [x] docstring for all new functions/methods/classes/modules.
  • [x] consistent style of all docstrings.
  • [x] for new modules: /docs/pages/modules.rst has been updated. (Not relevant I think)
  • [x] for api contributions: /docs/pages/api.rst has been updated.
  • [x] for api contributions: tutorials and examples have been updated.

Thank you and let me know if there's anything else I can improve on.

Best,

Roos

[piperfw]

Hi @rmadw! Great work on this. I've fixed two small things:

1. The docstring of compute_correlations (two-time) said it returned three items, but it now only returns two (this broke bath_dynamics.py line 133, but no other code was effected).
2. In the case of out of time order arguments, the progress bar wasn't being updated (try e.g. times_3 = 10*dt in the tutorial with the code before my edit)

Another thing I changed is to rename compute_nt_correlations to compute_correlations_nt, simply so it appears directly after compute_correlations in the documentation. If you have any reason this isn't a good idea, we can easily undo this (it's just my last commit there). I believe I've updated every instance of the function name in the code.

One thing I would want to be sure on is that the NaN's being returned when one has times out of order are correct. I suspect you've already checked this, in which case - nothing to do!

Finally, and this probably isn't something to be changed, it seems a little odd to me that we have to add +dt in times_1 = (0.0, dt*20+dt) to include the endpoint. Maybe there is a more intuitive way, but likely this is fine.

Given the above I am happy. Once Gerald has had time to check over things himself I'll complete the merge.

P.S. modules.rst is updated 'automatically' based on the docstrings in the code. So yes nothing more to do there either :)

[rmadw]

Hi @piperfw,

Thanks for the additional comments!

I didn't realise it was the compute_correlations code that was breaking bath_dynamics when I ran the test, sorry about that and thanks for fixing!

And all the other points sound good, I'm happy with renaming there. I've checked that all the NaN's are in the right places in the outputs, so I think that's all good.

I agree it's a bit annoying that you have to add a dt to the time inputs though, I think this might have do to do with how the parse_times function deals with the time inputs. I'll have a look at that later today to check if there's a good fix for that.

[gefux]

Hi @rmadw & @piperfw,

I apologise: I didn't realise that you've been waiting for my OK. I had a look through. It's great! I only have a few minor comments:

• The preprint reference is missing in: README.md, index.rst, HOW_TO_CITE.md, BIBLIOGRAPHY.md, how_to_cite.rst, bibliography.rst
• In the compute_correlations_nt the parameter naming ('dipole_ops') and the docstring references non-linear spectroscopy, although this is only a specific use-case of this general function. I think the parameter should thus be named 'operators' and the comment I'd remove the "-- written for non-linear spectroscopy purposes --" because this gives the user the (incorrect) impression that one shouldn't use it of other purposes.
• Similarly in the README.md and index.rst, it should say something like "...compute higher order multi-time correlations (e.g. for 2D electronic spectroscopy) [10]"
• In the tutorial: sometimes the units don't render correctly on my jupyter version. I'd use \mathrm{ps} and stay in the LaTeX environment, e.g. $\omega_{cutoff} = 3.04\,\mathrm{ps}^{-1}$, and $T=100\,\mathrm{K}=13.09 \frac{1}{\mathrm{ps \, k_B}}$.

I'll double check the NaNs on Monday, and get back to you.

[piperfw]

Hi Gerald, Thanks for looking over this. @rmadw would you be able to implement the suggested naming and description changes? Similarly with the tutorial rendering - I think it would be worth double checking the converted .rst version as well, because issues can arise there too.

Regarding the preprint reference, this was temporarily removed due to a change in state of preparation of the paper. @rmadw would you be happy for it to be included now, or prefer to delay until a later point? Again this version of the documentation won't be 'live' on readthedocs for some months probably.

[rmadw]

Hi Gerald,

Thanks a lot for the feedback! I'm currently implementing the changes you suggested.

As @piperfw mentioned, my preprint is still in draft stage and will hopefully be submitted soon, but I'm not completely confident what the timeline for that is. I'd prefer to delay adding the preprint reference for now if that's OK.

Let me know if there's anything else I can do! - Roos

[gefux]

Hi @rmadw and @piperfw:

I had now a closer look into the examples and the 'NaN's. The two time correlation 'NaN's don't seem to be quite correct. I have created a simple example and added the expected result (from previous implementation) as a comment at the end of the file:

• The equal-time operators should not be 'NaN' in 'ordered' mode and in the 'anti' ordered mode should give me the complement (in terms of the shape of the matrix) compared to the 'ordered' mode.
• Also a minor note: the progress_type doesn't get silent when set to 'silent'.

Also:

• In both, the tutorial and the performance example it would be more suitable to use the slices and indices to specify the time steps (in the same way as it's done in the simple example I just added).
• The example in example/nt_corrs_example.py has some global file references to your (rmadw's) hard disc, which clearly gives an error. I think the example is there in the performance section and the tutorial and can thus be removed from the example directory.

[rmadw]

Hi @gefux

Thank you for the additional feedback.

• I've almost fixed the code now for the 'ordered' vs 'anti' ordered correlations. The only issue remaining is that in the 'ordered' mode, the code will calculate correlations for t_a >= t_b, but in the 'anti' mode it will calculate t_a <= t_b instead of t_a < t_b (so matrices aren't exact complements). The solution I've implemented for now is to just remove any correlations from the array for which t_a==t_b, but will think a bit more whether there's a more elegant solution.
• All other feedback is being fixed right now :).

[piperfw]

Hi @rmadw Thanks for working on the changes. Regarding the ordering, can you explain why it is an issue to have symmetric behaviour here? Naively I would expect time ordering to mean t_A < t_B [other way around in your comment?] and anti-ordered to mean t_A > t_B, but wouldn't be surprised if both cases included equality. In other words, I would argue to keep the equal times (or not) in both cases.

Beyond that, if you would like to confirm you've addressed all of Gerald's feedback I can make the PR.

[rmadw]

Hi @piperfw,

I think I do agree with you there; the original two-time correlations code did implement the ordering as t_A <= t_B and t_A > t_B (sorry, I indeed switched the arguments by accident in my comment), which is why I raised it as an issue in my previous comment.

For my 2D spectroscopy related purposes, it would be useful to have the 'ordered' inequality set as t_A <= t_B, so I'd be happy keep the equal times in both inequalities if everyone is happy with that. In that case I'll remove the few lines I added to the code to remove the equal times.

Besides that, I can confirm that I've implemented all of Gerald's feedback!

[piperfw]

Thanks for confirming @rmadw and I think, please go ahead and remove those lines.

[rmadw]

No problem @piperfw, lines have been removed!

[piperfw]

That's complete, congratulations @rmadw !

Regarding your preprint, I think ultimately we should have a reference in the documentation here (and OQuPaper). I would suggest we either i. cite a work by you and BWL/JMJK 'in preparation' or ii. cite the oqupaper (once it is submitted). I would vote i.

A tentative date for dev0.5 to go live is 24th of this month, so one way or the other perhaps you could give us an update on the status of the preprint by then?