Merging coord_type into GeneralizedContractionShell

[maximilianvz]

Steps

• 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

Description

This pull request addresses what was requested in Issue #113. Thus far, I have pushed 6 commits to my forked repo, which I will describe below:

Add coord_type attr to GeneralizedContractionShell

This is perhaps the most fundamental commit in the PR. The commit message describes it pretty well: I have added the coord_type attribute to the GeneralizedContractionShell class. When creating an instance of the GeneralizedContractionShell class, one should pass a string that describes the coordinate type of the contraction. This string should either be "c"/"cartesian" to specify the cartesian coordinate type, or "p" (pure)/"spherical" to specify the spherical coordinate type. By default, "p" is provided, so the spherical coordinate type is assumed if nothing is passed for instantiation.

Negate from_iodata return of coord_types

With the addition of the coord_types attribute to GeneralizedContractionShell objects, there is no need for from_iodata to return coord_types, as the coord_types get embedded in the basis that the function returns. Of course, I needed to pass the coord_types to the IODataShell objects in this function to ensure my last point stands.

Update make_contractions

The make_contractions function is useful for instantiating a basis (list of GeneralizedShellContraction objects) for a system of atoms based on the basis_dict output of the parsers from gbasis.parsers. Previously, it only received the basis_dict, atoms at which the contractions are centered, and the coordinates of each atom. However, if we are to embed coord_types into the basis objects that we create, make_contractions must be able to handle that as an input as well. I have set this up such that, for a given call of make_contractions, one can pass either a string ("cartesian" or "spherical") or a list of strings to specify coord_types. If a string is passed, all contractions for all atoms are treated as having the same coord_type. However, if a list is passed, it must be the same length as the total number of contractions across all atoms. For example, if atom 1 contributes 4 contractions and atom 2 contributes 5 contractions, the coord_types list should be of length 9, where the first 4 elements correspond to contractions for atom 1 and the last 5 elements correspond to contractions for atom 2.

As I am writing this, I realize that maybe a user could try to pass "c" or "p" as a string in place of coord_types. Likely, they would do this expecting it to assign cartesian or spherical coordinate types to each contraction, respectively, though this would raise an error. This begs the question of whether I should change this commit by changing if coord_types == "spherical": to if coord_types in ["spherical", "p"]:, for example, or to change the allowed arguments for instantiation in the GeneralizedContractionShell class (this option requires me to alter my first commit described above).

Remove coord_type args from evals/integrals

This is the meat of where I had to deal with the fact that we no longer want to have to pass coord_types for eval/integral functions in our API. Essentially, if we embed coord_types in our basis variables (which are lists of GeneralizedContractionShell objects, now containing coord_type attributes), we should only have to pass basis to these functions, and then extract the coord_types from the contractions so that they can be passed into the construct_array* functions in the base*.py modules. Generally, I do this via coord_type = [type for type in [shell.coord_type for shell in basis]]. This is done internally, so the user need not worry about passing coord_types to these functions. In fact, if they try to pass coord_types, they'll encounter an error, as I removed the optional coord_type arguments from the eval/integral functions.

Update docs and coord_type checks in base*

Because I extract coord_type as a list prior to passing into the contruct_array* functions of the base*.py modules, construct_array_cartesian and construct_array_spherical don't actually get used based on the pre-existing checks. Previously, these functions would only be used if coord_types == "cartesian" or coord_types == "spherical", but if coord_types is a list, this obviously won't check out. Thus, to address this, I added checks that, for example, all(type == "cartesian" for type in coord_type) to see if construct_array_cartesian can be used. This raises an important point that I'd like to be addressed:

With coord_types being passed as a list, I could get away with just using construct_array_mix across the board (i.e., all the tests will pass). I don't know if there is a boost in efficiency that can be achieved by using construct_array_cartesian/spherical as opposed to construct_array_mix, but if there is not, I bet we could simplify the base*.py modules a lot by removing construct_array_cartesian/spherical and replacing construct_array_mix with construct_array, which would be able to handle homogeneous or heterogeneous coord_type lists equally well. I do realize that construct_array_lincomb makes calls to construct_array_cartesian/spherical though, so things would be a bit more complicated than what I've described. I didn't want to go that far in this PR, in case there's a benefit to maintaining the use of construct_array_cartesian/spherical, but it was an important point that I wanted to bring up in any case.

Adjust tests for coord_types API change

This one should be pretty self-explanatory. When I removed the coord_type optional arguments from the evals/integrals modules, any tests that called these functions would cause an error. In general, I just modified tests so that they didn't use coord_types as an argument for evals/integrals functions, but instead specified coord_types when creating a basis variable, which then gets passed to evals/integrals functionality.

One place where I sort of brute-forced the tests to work was when construct_array_lincomb was tested directly against some other functionality. Here's an example of what I mean:

You'll notice that at the bottom of that screenshot, I changed angular_momentum_integral_obj.construct_array_lincomb(transform, "spherical"), to angular_momentum_integral_obj.construct_array_lincomb(transform, ["spherical"]),. The change here is that "spherical" became ["spherical"] (I passed a list instead of a string). I call this "brute-forced" because, based on my changes in checks of coord_types in base*.py modules (see the description of the commit directly above this one), coord_types should be received in those modules as a list. When calling make_contractions, as I described above, you don't have to pass coord_types as a list - if you pass a string it'll work and treat all contractions with the same coordinate type. Based on that, one might want to pass coord_types to construct_array_lincomb as a string in tests. However, I don't believe users should be directly accessing functions in the base*.py modules, so only using this "brute-forced" change of passing a list versus a string should hopefully be okay just in the context of testing.

Type of Changes

	Type
✓	🐛 Bug fix
✓	✨ New feature
✓	🔨 Refactoring
✓	📜 Docs

Related Issue

[FarnazH]

@maximilianvz I noticed that tests/test_eval.py::test_evaluate_basis_pyscf fails. I removed the argument coord_type in 5a52509, but it still failed because the from_pyscf uses spherical coord_type by default. Can you fix this?

This makes me also wonder whether the coord_type argument of GeneralizedContractionShell should be an optional argument (instead of being required). Did you choose the default value of “p” because the evaluate* functions had such defaults?

Thanks for your detailed description in this PR. I appreciate the effort you have put into explaining the changes you have made. For the most part, you don’t need to explain in words the changes you made to the code. The reviewer is familiar with the code, and considering that your commits are bit-sized, it should be clear to the reviewer what has happened. This can save you a lot of time in making a PR. For example, looking at Adjust tests for coord_types API change, the changes you have made and why you have made them is clear because you have made a good commit fixing one thing. If anything is unclear the reviewer can ask. I recommend only explaining your logic when it is not obvious from the code.

Instead, it is more useful to add these descriptions to the

1. body of your commit messages (leave an empty line after your commit message/title and then include a body), because then your description stays with the commit and is easier to refer to (than finding the PR where the changes were made). Like 5a52509

2. Or, sometimes to the docstring. See my commit 6109e8d.

In QC-Devs packages, we try not to provide the default value in the docstring. See 6109e8d. The default is visible in def ... line. This makes the doctoring shorter and easier to maintain (it can easily go unnoticed if someone changes the default value but don’t change the docstring).

In Remove coord_type args from evals/integrals,
type is a built-in function name, so it should not be used as a variable name. your editor should color code that. Change the variable name (e.g., to ct).
What if coord_type is “c” or “p”?

[maximilianvz]

@FarnazH, I believe I've addressed both of the changes you asked for:

1. I got tests/test_eval.py::test_evaluate_basis_pyscf to pass by requiring that the user pass the coordinate types of shells when making a basis list via from_pyscf. Using from_iodata, there was an elegant way to extract this information from mol (such that the user didn't have to specify coord_types in the call to the function), but I don't know if this is possible with from_pyscf, so I went with this workaround.
2. I stopped using the reserved type keyword inappropriately, as you pointed out.

Addressing your comments:

1. I doubt it would be very difficult to make coord_type a required argument for GeneralizedContractionShell. You are correct in guessing that I used a default value of p because the eval* functions were doing this previously.
2. In general, if the user is using make_contractions or from_pyscf to build a basis list, they should only pass "spherical", "cartesian", or a list of these strings (actually, now that I write this, I realize you could pass a list of "c"s and "p"s, which slipped under my radar). Passing "c" or "p" individually is technically allowed if you're accessing GeneralizedContractionShell directly, but these higher-level functions don't recognize those as valid inputs. Evidently, there's an inconsistency here. I can address this by including the possibility of passing c and p as single strings to make_contractions and from_pyscf. Let me know if you have other ideas.

[FarnazH]

Thanks @maximilianvz. Based on your comments,

1. Let's make coord_type a required argument for GeneralizedContractionShell.
2. Let's make sure higher level functions support 'c' and 'p' as well.

[FarnazH]

@maximilianvz, the problem with popping elements from coord_types list is that, if there is more than 1 atom in the molecule, the coord_types list will be empty after the shells for the first atom are appended to basis. This tentative bug was not caught by the existing tests, because they all had a string coord_type.

Considering the recent change in 729d83e, and the fact that PySCF does not support mixed coordinate types (based on my quick look), testing this can be tricky (any thoughts?). Then, we should either only support str type coord_types or index the coord_types list (instead of popping). The latter is less desitable, because I am not still sure how to test a list coord_types.

@leila-pujal, @RichRick1, @msricher, and @PaulWAyers, do you know whether PySCF supports mixed "cartesian" and "spherical" basis?

[leila-pujal]

From what I remember using PySCF, it does not allow for mixed cartesian/spherical. There's an attribute in the Mole class called cart that's boolean. True the basis is cartesian and False spherical.

[maximilianvz]

@FarnazH, if PySCF doesn't allow for mixed cartesian/spherical coordinates, then we should remove the elif type(coord_types) == list: clause altogether, as the only time you would want to pass a list here is if you were using a mix of these coordinate systems. By adding this line, you prevent coord_types from being a list anyway. Thus, I don't see any "bug" applying to from_pyscf if we were to remove that part of the code.

However, make_contractions does allow for mixed coordinate systems. It also uses this elif type(coord_types) == list: clause, but I believe this test shows that popping from the coord_types list will work. In that example of a Krypton dimer, each Krypton atom contributes 5 shells, for a total of 10 shells. In the call to make_contractions, a list of length 10, where all but the last entries are "spherical", is passed. You will only ever pop from coord_types if the initial length of coord_types is equal to the TOTAL number of contractions in the molecule. In this case, the onus is on the user to specify a list of length 10 to make_contractions, and to ensure that they know that the first 5 coordinate types in this list would get applied to the 5 shells of the first Krypton atom, while the last 5 coordinate types in that list (where the last element was specified as "cartesian") would get applied to the five shells of the second Krypton atom.

Please let me know if you have any questions or doubts.

[FarnazH]

@maximilianvz, based on our discussions, I made the changes below. I believe this is good to be merged, let me know if you have any comments.

Take a look at the commits on how the code is refactored to avoid nested if/elif/else statements. Also, when checking the type of a variable, it is more appropriate to use isinstance function.