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

Standardize Libcint implementation to include IOData conventions #174

Merged
merged 23 commits into from
Apr 23, 2024

Conversation

leila-pujal
Copy link
Collaborator

@leila-pujal leila-pujal commented Apr 18, 2024

This PR builds on top of the recently merged Libcint interface and adds the necessary code to use libcint with data load through from_iodata wrapper. Before this update, the interface called libcint library followed default Gbasis conventions for the ordering of atomic basis functions. However, this default convention is not followed by other common wavefunction files (e.g fchk). This caused a mismatch ordering between arrays generated by Gbasis and Cbasis(Libcint) when using IOData with a wavefunction file that does not follow this default conventions. The PR includes the following code to fix this:

  • New method in IODataShell class permutation_libcint. This method is called by Cbasis class to permut the order of basis functions.
  • Change in the default ordering for p angular momentum for spherical functions: When basis information is loaded using basis set files they do not have associated ordering of the generated atomic basis functions (because they are not associated with a wavefunction calculation). Gbasis needs a convention to generate the atomic basis function from each shell. Particularly, for spherical atomic basis functions the convention is defined here and it uses cosine (c{m}, m=magnetic quantum number) and sine(s{m}, m=magnetic quantum number) nomenclature and for each angular momentum it orders by default first sine functions followed by cosine functions. For spherical p orbitals, the ordering would be s1,c0,s1 which corresponds to yzx. Before this PR the libcint library was compiled to have this output for the p orbitals. Keeping this ordering and at the same time using IOData results in an incompatibility problem at least for fchk format. Conventions stored in IOData for fchk only contain p cartesian conventions (because cartesian/spherical coordinates generate the same number of p atomic orbitals). This means even for all spherical shells calculation the conventions passed to Gbasis from IOData will contain p cartesian ordering, meaning xyz which corresponds to c1,s1,c0. Even having the permutation function because internally is stored as cartesian it won't change the ordering but for libcint it will call the spherical ordering which is zyx. To summarize keeping this default ordering you can not use a fchk because it is "missing" the spherical p angular momentum convention and thus we can not do the permutation. I hope this explanation is clear enough.

The PR includes tests for using IOData + libcint and for the change in the default spherical p orbital convention. For testing the integrals the electron-electron repulsion is skipped because of the high angular momentum included in the calculations. I am assuming that because for the other integrals, it works it should work for the electron repulsion too.

Checklist

  • Write a good description of what the PR does.
  • Add tests for each unit of code added (e.g. function, class)
  • Update documentation
  • Squash commits that can be grouped together
  • Rebase onto master

Type of Changes

Type
✨ New feature
🔨 Refactoring

Related

@leila-pujal
Copy link
Collaborator Author

@PaulWAyers, @FarnazH, me and @msricher have been working on this PR. I would like your opinion on changing the conventions of the spherical atomic basis functions for p orbitals if you think there is something I might have missed considering. Besides that, all the tests pass, and @msricher and I are reviewing the code so you don't need to go into detail if you don't have the time. If you think this is good, approve your review so I can merge the PR (I need 1 approved from reviewers with access). Thanks

Copy link
Contributor

@msricher msricher left a comment

Choose a reason for hiding this comment

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

Everything seems fine to me and the tests pass.

I do notice a problem with failing tests though. It seems none of the Github Actions workflows actually build Libcint? I wonder if this was ever a problem before... Can you add a line to the workflows to tun the tools/install_libcint.sh script?

Copy link
Member

@PaulWAyers PaulWAyers left a comment

Choose a reason for hiding this comment

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

I think it is OK to reorder the p functions. PySCF also uses xyz as an exception to their usual rule, so this seems reasonably common.

So let's chalk this up to "a foolish consistency is the hobgoblin of little minds" and do what's works best.

@leila-pujal
Copy link
Collaborator Author

Thanks @PaulWAyers and @msricher for your replies. I made changes to run the libcint tests when these are tested on ubuntu. For Windows, I didn't know how to install the lisp interpreter. To be honest I am not sure libcint can be installed on Windows. Let me know if you have more infor about this @msricher . I setup the test to run all of them on ubuntu but for Windows we don't run libcint tests. This caused the coverage to drop to 79% and that's why you see I changed the coverage. For ubuntu, where the libcint tests are run, the coverage is 96.80%. Besides pytest workflows, then there is the pre-commit bot that fails for ruff. @Ali-Tehrani was already aware and I am planing to take care of this soon. If no one has more revisions to do I will merge this PR tomorrow.

@msricher
Copy link
Contributor

Libcint does not support Windows.

@leila-pujal leila-pujal merged commit bb86ed6 into theochem:master Apr 23, 2024
10 of 11 checks passed
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.

3 participants