TREX-CoE / trexio

TREX I/O library
https://trex-coe.github.io/trexio/
BSD 3-Clause "New" or "Revised" License
49 stars 14 forks source link

Improvement on state #119

Closed scemama closed 1 year ago

scemama commented 1 year ago
  1. The state id is used as an index for the file names, so it is preferable to use state_id as an index instead of an integer.
  2. I added energy to store the energy of the state
q-posev commented 1 year ago
  1. Please do not forget to update the Python API version when you change it in configure/CMakeLists
  2. Why do you want the state_id to be index? We say explicitly that id=0 is ground state in the format definition. Thus, making it index will break this. Plus, this might cause the confusion cause the files accessed via Fortran API will have state_id=1 for ground state but corresponding to a file called e.g. trexio_0 with the ground state (if the file was produced by C), so accessing it programmatically will result in errors.
  3. Have you tested it? I do not think that index type works for single numbers.
scemama commented 1 year ago

Please do not forget to update the Python API version when you change it in configure/CMakeLists

OK

Why do you want the state_id to be index? We say explicitly that id=0 is ground state in the format definition. Thus, making it index will break this. Plus, this might cause the confusion cause the files accessed via Fortran API will have state_id=1 for ground state but corresponding to a file called e.g. trexio_0 with the ground state (if the file was produced by C), so accessing it programmatically will result in errors.

I was just implementing this in QP for multi-state calculations. I am writing files in Fortran, and reading the produced files in Python.

I got very confused: In the Fortran world, it is natural to consider the ground state as state 1, and the first excited state as state 2, because these are the indices used for that in all the arrays that contain multiple states. For example, I always have loops like:

do k=1,N_states
   array(:,:,k) = ...
end do

so it is natural to set the state id as k. And in the Python world, it was natural to have them as 0 and 1. It appeared to me that here we have an exception with no particular reason, and users will have this wrong because they don't read the documentation ;-)

Also, it is the index in the arrays label and file_name, and we need to shift the id by one.

Have you tested it? I do not think that index type works for single numbers.

Oh no! you are right, it doesn't work. I will fix it.

q-posev commented 1 year ago

I got very confused: In the Fortran world, it is natural to consider the ground state as state 1, and the first excited state as state 2, because these are the indices used for that in all the arrays that contain multiple states. For example, I always have loops like:

I see what you mean, but this is related to QP. There might be codes (e.g. linear response TD-DFT) which store info about their excited states in a separate array, so there state_id=1 would be the first excited state in their Fortran array, but in the index world it will correspond to state_id=2. There is probably no silver buller for it.

scemama commented 1 year ago

Now index type it works with scalar values :-)