Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement on state #119

Merged
merged 2 commits into from
May 12, 2023
Merged

Improvement on state #119

merged 2 commits into from
May 12, 2023

Conversation

scemama
Copy link
Member

@scemama scemama commented May 11, 2023

  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
Copy link
Member

q-posev commented May 12, 2023

  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
Copy link
Member Author

scemama commented May 12, 2023

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
Copy link
Member

q-posev commented May 12, 2023

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
Copy link
Member Author

scemama commented May 12, 2023

Now index type it works with scalar values :-)

Copy link
Member

@q-posev q-posev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@scemama scemama merged commit 16b9e0b into master May 12, 2023
@q-posev q-posev deleted the state branch June 16, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants