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

A new LibXC interface based on CFFI and new MC-DCFT code #70

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

Dayou-Zhang
Copy link

This pull request consists of two contributions:

  1. It implements a new LibXC interface.
    The new interface uses the CFFI library to call LibXC. It is located in a separate package named dft2. The new interface is fully backward compatible with the current interface as demonstrated by the test cases. It is implemented based on the PySCF 2.7 LibXC interface code. Compared to the current LibXC interface, the new interface has several advantages:
  • All underlying LibXC data structure are now cached in the Python instead of in the C library. This allows direct Python access to the underlying LibXC data structure, which provides users with more flexibility in case they need access to LibXC features for which PySCF have not yet implemented an interface.
  • As a result, most code from lib/dft/libxc_itrf.c are not needed and can be replaced by a Python equivalent, as performed in this pull request. This makes it easier for future interface development, in case LibXC introduces new features such as double hybrid functionals as mentioned in double hybrid functional #52.
  • It provides an interface to set external parameters of LibXC functionals. This is one of the key motivations of this pull request because many of our work involves setting functional parameters, which PySCF currently does not support this feature yet.
  • The current DFT interface uses a string to represent a density functional in the Python layer and construct LibXC data structure in C for every API call. As a result, the LibXC data structures are constructed and destructed many times during a DFT calculation, which adds on some overhead. This can be avoided by caching the LibXC data structure in Python and reusing them throughout the entire calculation. In the new interface, a class named XCFunctional is provided, which is intended to be served as a high-level API representing an exchange–correlation functional, and perhaps potentially be used to replace the xc_code string in DFT codes to enhance performance. The design of the high-level API is still not mature, so I am open to suggestions from the PySCF community.
  1. It includes the code reflecting the latest development of MC-DCFT.
    This pull request enhances the code for multiconfiguration density coherence functional theory (MC-DCFT, doi: 10.1021/acs.jctc.0c01346) by integrating with the abovementioned LibXC interface. It allows users to perform calculations with our recently developed density coherence functional DC24 (doi: 10.26434/chemrxiv-2024-78tt8).

@sunqm
Copy link
Contributor

sunqm commented Sep 27, 2024

I took a quick look at the cmake part, and I noticed it adds a CFFI build section. Is the CFFI build necessary? Using CFFI for the interface would require building a wheel for each Python version, which adds efforts to devops maintenance.

@sunqm sunqm self-requested a review September 27, 2024 04:46
Dayou-Zhang and others added 2 commits September 27, 2024 12:35
…wheels for cp37, which makes HDF5 backend missing for trexio"

This reverts commit fe54aa1.
@MatthewRHermes
Copy link
Collaborator

  1. It looks like pyscf/dft2/libxc.py and pyscf/lib/dft/libxc_itrf2.c (at least) originate from copy-pasted source files in the PySCF core modules. Could you add a comment to the top of both files identifying the file you copied, the PySCF core-module commit from which you made the copy, and describing briefly the modifications? This will make it easier to debug in the future when continued updates to the PySCF core modules will inevitably need to be reproduced.
  2. I would recommend adding an example script and unittests in which you modify functional parameters using this interface (i.e., the basic reason that you had to rewrite libxc.py), other than just checking DC24.
  3. I itch a little seeing a .so file anywhere other than the pyscf/lib folder. Is there a compelling reason that libxc_cffi.*.so can't go there?

@MatthewRHermes
Copy link
Collaborator

I took a quick look at the cmake part, and I noticed it adds a CFFI build section. Is the CFFI build necessary? Using CFFI for the interface would require building a wheel for each Python version, which adds efforts to devops maintenance.

If I understand correctly, the use of CFFI is basically an alternative to writing several new functions in `libxc_itrf.c', as in the final version of this PR.

@Dayou-Zhang
Copy link
Author

I took a quick look at the cmake part, and I noticed it adds a CFFI build section. Is the CFFI build necessary? Using CFFI for the interface would require building a wheel for each Python version, which adds efforts to devops maintenance.

The current implementation is performed in the CFFI API mode, which requires a CFFI build. If we re-write in ABI mode, it will probably not require a build, but it is not the recommended way according to the manual, as ABI mode is slower and is more prone to problems. Therefore, I personally prefer API mode.

  1. It looks like pyscf/dft2/libxc.py and pyscf/lib/dft/libxc_itrf2.c (at least) originate from copy-pasted source files in the PySCF core modules. Could you add a comment to the top of both files identifying the file you copied, the PySCF core-module commit from which you made the copy, and describing briefly the modifications? This will make it easier to debug in the future when continued updates to the PySCF core modules will inevitably need to be reproduced.

This is a good suggestion. I added the comments in the files that are adapted from PySCF core modules.

  1. I would recommend adding an example script and unittests in which you modify functional parameters using this interface (i.e., the basic reason that you had to rewrite libxc.py), other than just checking DC24.

There is already one test case in dft2/test/test_libxc_functional.py where B97-1 and B97-3 XCFunctional objects are set to the parameters of B97-2. Currently I cannot think of a useful example script to add because we have not integrated the interface with other modules (such as Kohn-Sham module) yet, and the MC-DCFT module already has a straightforward high-level API for setting functional parameters.

  1. I itch a little seeing a .so file anywhere other than the pyscf/lib folder. Is there a compelling reason that libxc_cffi.*.so can't go there?

The reason I prefer to put libxc_cffi.*.so in dft2 is because one can easily do from .libxc_cffi import ffi as _ffi, lib as _lib in libxc.py. It is not loaded using lib.load_library like other ctypes libraries. We can put it in lib, but it is less straightforward to import the library. I have seen some Python libraries (e.g., numpy and scipy) mixing pure python code and .so files, so I am not sure if it is preferable to put all .so files in lib.

@Dayou-Zhang
Copy link
Author

I have now implemented the interface using the CFFI ABI mode, which should no longer need a separate wheel for each Python version. This will be the default mode when users install the package with pip install. However, users still have the option to compile the CFFI library using API mode from source if they pass the -DBUILD_CFFI_API_MODE=ON option to cmake.

@MatthewRHermes
Copy link
Collaborator

  1. I would recommend adding an example script and unittests in which you modify functional parameters using this interface (i.e., the basic reason that you had to rewrite libxc.py), other than just checking DC24.

There is already one test case in dft2/test/test_libxc_functional.py where B97-1 and B97-3 XCFunctional objects are set to the parameters of B97-2. Currently I cannot think of a useful example script to add because we have not integrated the interface with other modules (such as Kohn-Sham module) yet, and the MC-DCFT module already has a straightforward high-level API for setting functional parameters.

OK, but can you add an example script for the use of that straightforward high-level API?

@Dayou-Zhang
Copy link
Author

  1. I would recommend adding an example script and unittests in which you modify functional parameters using this interface (i.e., the basic reason that you had to rewrite libxc.py), other than just checking DC24.

There is already one test case in dft2/test/test_libxc_functional.py where B97-1 and B97-3 XCFunctional objects are set to the parameters of B97-2. Currently I cannot think of a useful example script to add because we have not integrated the interface with other modules (such as Kohn-Sham module) yet, and the MC-DCFT module already has a straightforward high-level API for setting functional parameters.

OK, but can you add an example script for the use of that straightforward high-level API?

Yes, I just added an example script to show how users can set functional parameters using the high-level API in MC-DCFT.

@matthew-hennefarth
Copy link
Contributor

Can you also add the meta-GGA functional available for MC-PDFT as well? This way OpenMolcas (with this PR) and PySCF would be up to date with regards to functional functionality.

@MatthewRHermes
Copy link
Collaborator

Failing unit tests are due to #71, cf. #72

@sunqm
Copy link
Contributor

sunqm commented Oct 20, 2024

After carefully consideration, I strongly recommend removing the dependency on cffi as it introduces unnecessary complexity. While using ctypes may be not as elegant as cffi, it is sufficient to provide the functionality here. It is not a bottleneck here and it aligns better with our current architecture. A straightforward and stupid solution is better for long-term manageability.

@MatthewRHermes
Copy link
Collaborator

  1. It looks like pyscf/dft2/libxc.py and pyscf/lib/dft/libxc_itrf2.c (at least) originate from copy-pasted source files in the PySCF core modules. Could you add a comment to the top of both files identifying the file you copied, the PySCF core-module commit from which you made the copy, and describing briefly the modifications? This will make it easier to debug in the future when continued updates to the PySCF core modules will inevitably need to be reproduced.

This is a good suggestion. I added the comments in the files that are adapted from PySCF core modules.

Could you describe your modifications and report the commit hash from which you copied those files in those comments?

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.

4 participants