Store dimensions (orbital matrix, CI vectors) in Checkpoint.txt

[eirik-kjonstad]

Suggestion to store the dimensions of the CI vectors and orbital coefficient arrays in Checkpoint.txt.

Currently, the Checkpoint.txt will dump the vectors only, but this means that OpenFMS must know upon restart what the lengths of these vectors are. This is awkward since it requires "early" communication with the electronic structure backend upon restart that is not needed in a normal run, in particular, we must 1) ask backend for dimensions; 2) read checkpoint; 3) ask backend for electronic structure at restart geometry. If we add the dimensions to Checkpoint.txt, then we can instead have a simpler sequence: 1) read checkpoint; 2) ask backend for electronic structure at restart geometry.

I guess that for other electronic structure backends, it might be reasonable to keep "initialize_interface", but it seems at least that dimensions do not need to be resolved here. In the case of eT, the interface only implements RuneT and does not have any need for initialization.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 47.76119% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.68%. Comparing base (cd33087) to head (244d7da).

Files with missing lines	Patch %	Lines
src/modules/RestartModule.f90	47.76%	29 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
- Coverage   53.81%   53.68%   -0.14%     
==========================================
  Files          41       41              
  Lines        6000     6058      +58     
  Branches      812      825      +13     
==========================================
+ Hits         3229     3252      +23     
- Misses       2420     2449      +29     
- Partials      351      357       +6     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[danielhollas]

Thanks! Seems reasonable, I'll review later this week.

[danielhollas]

Generally looks great, mostly have just some questions. I'll later also test with TeraChem backend.

We generally don't have much experience with the restart code path. I have not personally validated it and Basile has always been very skeptical of the restart functionality. Do you have experience with it? Did it work for you fine?

[danielhollas]

Suggested change

	read (nf, '(A)', iostat=ierr) header
	if (ierr /= 0) call FMS_DieError('Error reading electronic-structure restart header')
	read (nf, '(A)') header

Let's remove iostat and let the program crash on error, the current code would hide the error message.

[eirik-kjonstad]

Changed the reads throughout to crash instead of checking iostat

[danielhollas]

So maybe I am misunderstanding the notation, but is this format correct? Will it correctly read variable-sized matrix?

[eirik-kjonstad]

I think it is correct but we are writing/reading single-precision here, so could possibly be changed throughout to something like es24.16e3 to preserve double-precision. This is using so-called format reversion, which means the matrix would be printed with 10 entries per line until all elements of the matrix are read/written (see https://stevelionel.com/drfortran/2009/07/01/doctor-fortran-in-revert-revert-the-end-of-the-format-is-nigh/). If we want to make things clearer we could do an explicit loop with a parameter n_entries_per_line = 10.

[danielhollas]

Huh, interesting, had no idea about this behaviour, thanks for the link! If this works for you let's keep it like this for now, we can adjust later.

[danielhollas]

Here's again, let's crash hard, or print the errmsg otherwise it's not clear what was wrong.

[eirik-kjonstad]

Hopefully fixed

[eirik-kjonstad]

Generally looks great, mostly have just some questions. I'll later also test with TeraChem backend.

Thanks for the review! I'll try to get to these in the next couple of days.

We generally don't have much experience with the restart code path. I have not personally validated it and Basile has always been very skeptical of the restart functionality. Do you have experience with it? Did it work for you fine?

We have some experience with it, and, as far as we know, it works as intended. I have a few tests which compare restarted runs with full runs (both with and without spawning), and I believe the results are identical when restarting. I'll try to run some more tests to confirm.

[danielhollas]

@eirik-kjonstad thanks! I am happy to merge this. Just to double check my understanding of the code, this should be backwards compatible, right? If the new code tries to restart from the old format it should still work.

I unfortunately don't have time to test this at this point, but since it works for you happy to merge, and once I get to it I'll do a follow-up if I run into any issues.

[eirik-kjonstad]

@eirik-kjonstad thanks! I am happy to merge this. Just to double check my understanding of the code, this should be backwards compatible, right? If the new code tries to restart from the old format it should still work.

I unfortunately don't have time to test this at this point, but since it works for you happy to merge, and once I get to it I'll do a follow-up if I run into any issues.

Yes, that is the intent. The new reader accepts both formats: if # Orbitals / # CI vectors include dimensions, it uses those; if the headers do not include dimensions, it falls back to the backend-initialized esNBasis / esLCIVec. So old TeraChem restart files should remain readable in the normal startup path, where TeraChem initializes those sizes before the restart bundle is read. Unfortunately, I don't have TeraChem and so cannot verify this.

[bcurchod]

Generally looks great, mostly have just some questions. I'll later also test with TeraChem backend.

We generally don't have much experience with the restart code path. I have not personally validated it and Basile has always been very skeptical of the restart functionality. Do you have experience with it? Did it work for you fine?

Sorry to be late to the party! Just as a note: one potential issue with restarts was the diabatization of the orbitals between time steps to check for missed intersections/avoided crossings by comparing CI vectors (and also to ensure that the 'phase' of the electronic wavefunctions is continuous). It was important to ensure that all necessary information was stored for a proper restart. I would need to check what the current status is.

[eirik-kjonstad]

Generally looks great, mostly have just some questions. I'll later also test with TeraChem backend.
We generally don't have much experience with the restart code path. I have not personally validated it and Basile has always been very skeptical of the restart functionality. Do you have experience with it? Did it work for you fine?

Sorry to be late to the party! Just as a note: one potential issue with restarts was the diabatization of the orbitals between time steps to check for missed intersections/avoided crossings by comparing CI vectors (and also to ensure that the 'phase' of the electronic wavefunctions is continuous). It was important to ensure that all necessary information was stored for a proper restart. I would need to check what the current status is.

@bcurchod Thanks for the note! For the diabatization of orbitals/phasing of CI vectors, we handle this on the eT-side and pass consistently phased quantities back to FMS. On restart the eT-side phasing is relative to the orbitals/CI-vectors read from Checkpoint.txt.