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

Update of PySCF checkpoint files for the GitHub action #32

Merged
merged 6 commits into from
Jan 18, 2024

Conversation

kousuke-nakano
Copy link
Member

@kousuke-nakano kousuke-nakano commented Dec 20, 2023

We have found a backward incompatibility of the PySCF checkpoint files. So, I have made *_v2.3.chk, and *_v2.4.chk.

Dear @q-posev, do you think if we can insert some if~else sentences depending on an installed PySCF version in the test.yml?

Kosuke Nakano

EP

…s. So, I have made *_v2.3.chk, and *_v2.4.chk.
@kousuke-nakano kousuke-nakano added the bug Something isn't working label Dec 20, 2023
@kousuke-nakano kousuke-nakano self-assigned this Dec 20, 2023
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.

Dear @kousuke-nakano,

I don't think the bug that I reported has been fixed. The source code of the PySCF converter has not been modified, which means that it is still incompatible with files prodiced by PySCF v.2.4. Is it a lot of effort to make it compatible with the latest PySCF?

I believe that we should execute different converter code depending on the PySCF version. That's the best way to make converter compatible with all versions of PySCF. Adapting CI is a workaround, not a solution to me.

@kousuke-nakano
Copy link
Member Author

Dear @q-posev, I believe it has been fixed. I have confirmed that the trexio converter works both with PySCF v2.3.0 and v2.4.0 (Of course, one should read a corresponding checkpoint file, *_v2.3.chk or *_v2.4.chk). The CI failed just because I have changed the checkpoint filenames. I do not think we have to modify the source code of the PySCF converter because the error comes from a line reading a PySCF checkpoint file, which does not affect the implementation of the PySCF converter.

@q-posev
Copy link
Member

q-posev commented Dec 20, 2023

I must have misunderstood then, my bad. So what you are saying is that:

  1. Having PySCF v.2.4 installed in the environment and reading the .chk file produced by PySCF v.2.4 works
  2. Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF <= v.2.3 works
  3. Having PySCF v.2.4 installed in the environment and reading the .chk file produced by PySCF <= v.2.3 fails
  4. Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF v.2.4 fails

Is that correct?

@q-posev
Copy link
Member

q-posev commented Dec 20, 2023

Maybe there is a way to detect the version of PySCF used to produce the .chk file? This would help fix the converter. Adding suffix is not really bulletproof, the user might have .chk without knowing the PySCF version that produced it.

@kousuke-nakano
Copy link
Member Author

kousuke-nakano commented Dec 20, 2023

Dear @q-posev

I must have misunderstood then, my bad. So what you are saying is that:

Having PySCF v.2.4 installed in the environment and reading the .chk file produced by PySCF v.2.4 works
Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF <= v.2.3 works
Having PySCF v.2.4 installed in the environment and reading the .chk file produced by PySCF <= v.2.3 fails
Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF v.2.4 fails
Is that correct?

Yes, exactly!

Maybe there is a way to detect the version of PySCF used to produce the .chk file? This would help fix the converter. Adding suffix is not really bulletproof, the user might have .chk without knowing the PySCF version that produced it.

You are right. Let me think about it.

@kousuke-nakano
Copy link
Member Author

kousuke-nakano commented Jan 8, 2024

Dear @q-posev and @scemama, happy new year. Sorry for my long silence. I am back to work. I have noticed that Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF v.2.4 works. So, the simplest solution is to replace the old PySCF v.2.3 files with the ones produced by PySCF v.2.4. It works.

The CI failed because the following two lines did not work

  • trexio convert-to -t cartesian -o trexio_orca_h2o.h5 trexio_orca_h2o_sph.h5
  • trexio convert-to -t cartesian -o trexio_pyscf_h2o.h5 trexio_pyscf_h2o_sph.h5

@q-posev
Copy link
Member

q-posev commented Jan 10, 2024

Dear @kousuke-nakano ,

Happy New Year!

So from what I understand we now have:

  • Having PySCF v.2.4 installed in the environment and reading the .chk file produced by PySCF v.2.4 works
  • Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF <= v.2.3 works
  • Having PySCF v.2.4 installed in the environment and reading the .chk file produced by PySCF <= v.2.3 fails
  • Having PySCF v.2.3 installed in the environment and reading the .chk file produced by PySCF v.2.4 works [updated]

@kousuke-nakano do you know what has changed between 2.3 and 2.4 in .chk file format? If they added/removed some data fields - we can use them to detect which version of PySCF produced the .chk. Unfortunately, they do not store PySCF version in the metadata (at least I did not find it).

@scemama the ORCA or PySCF spherical->cartesian conversion fails in the CI following recent additions introduced by @joguenzl in MR #30 . Basically, we now always attempt to read basis_r_power, but this property is missing in the TREXIO file produced by the converter. Should it always be present in the TREXIO files? If so, we may need to adapt the converters, otherwise trexio_read_basis_r_power call should be made optional.

@joguenzl
Copy link
Contributor

To my understanding, basis_r_power should always be adapted during the conversion since for spherical harmonics, the angular part is completely separated from the radius, whereas the cartesian polynomials mix the angular and radial coordinates, so they need to be divided by r to make them homogeneous again.

I think it would thus make sense to set basis_r_power to an array of zeros if it does not exist in the input file and continue the conversion as implemented from there.

@kousuke-nakano
Copy link
Member Author

Dear @q-posev

@kousuke-nakano do you know what has changed between 2.3 and 2.4 in .chk file format? If they added/removed some data fields - we can use them to detect which version of PySCF produced the .chk. Unfortunately, they do not store PySCF version in the metadata (at least I did not find it).

I do not think they have changed the format between 2.3 and 2.4. The error we found seems a bug in PySCF <= v2.3, where False is stored as false. I am not sure why this error was not problematic in the previous version...

Traceback (most recent call last):
File "/home/runner/.local/bin/trexio", line 8, in
sys.exit(main())
File "/home/runner/.local/lib/python3.10/site-packages/trexio_tools/trexio_run.py", line 95, in main
run(args["TREXIO_FILE"], args["--input"], args["--type"], back_end=back_end, motype=args["--motype"])
File "/home/runner/.local/lib/python3.10/site-packages/trexio_tools/converters/convert_from.py", line 770, in run
run_pyscf(trexio_filename=trexio_filename, pyscf_checkfile=filename, back_end=back_end_str)
File "/home/runner/.local/lib/python3.10/site-packages/trexio_tools/converters/pyscf_to_trexio.py", line 37, in pyscf_to_trexio
mol = scf.chkfile.load_mol(pyscf_checkfile)
File "/home/runner/.local/lib/python3.10/site-packages/pyscf/lib/chkfile.py", line 172, in load_mol
moldic = eval(fh5['mol'][()])
File "", line 1, in
NameError: name 'false' is not defined. Did you mean: 'False'?

Anyway, since the format is not changed, we can safely use the .chk files generated by the latest version.

@scemama
Copy link
Member

scemama commented Jan 11, 2024

I fixed the basis_r_power issue in #33

@q-posev
Copy link
Member

q-posev commented Jan 11, 2024

@kousuke-nakano if it crashes, then it means that PySCF changes smth in the way they read the CHK files. So basically if the file came from <=2.3 - the user would need to downgrade their PySCF version... Otherwise, we are good. I will think if there is better solution.

@scemama do we want to fix it at the Q-Chem code converter level? PySCF converter does not write the r_power neither so the spherical->cartesian conversion will fail for it too. That's why we need to decide now: either we do as @joguenzl suggests and set r_power to zeros if it is not present in the input file inside the spherical->cartesian converter (makes sense to me) OR we need to modify each converter to make sure it writes zero-ed r_power array.

@scemama
Copy link
Member

scemama commented Jan 11, 2024

@scemama do we want to fix it at the Q-Chem code converter level? PySCF converter does not write the r_power neither so the spherical->cartesian conversion will fail for it too. That's why we need to decide now: either we do as @joguenzl suggests and set r_power to zeros if it is not present in the input file inside the spherical->cartesian converter (makes sense to me) OR we need to modify each converter to make sure it writes zero-ed r_power array.

@q-posev I agree with @joguenzl that we should always write r_power = [0., ..., 0.].

@q-posev
Copy link
Member

q-posev commented Jan 18, 2024

Dear @kousuke-nakano ,

We fixed issues with basis_r_power in both pyscf and orca converters in fix_orca branch. I also pinned pyscf version to 2.3 for testing since you told me that it should work with files produced by pyscf 2.4.

Could you please merge changes from the fix_orca branch in your branch and push so that we can test that this MR is green with your new pyscf test files?

@kousuke-nakano kousuke-nakano marked this pull request as ready for review January 18, 2024 11:06
@kousuke-nakano
Copy link
Member Author

Dear @q-posev, OK! This MR is green!

@q-posev
Copy link
Member

q-posev commented Jan 18, 2024

@kousuke-nakano thanks for the update! I will merge this MR after #33.

@q-posev q-posev merged commit 912cbd4 into master Jan 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PySCF to TREXIO converter is broken
4 participants