Rename nprim to nexp where appropriate
#357
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
|
@sourcery-ai review |
|
Thanks for reviewing Paul! |
Reviewer's Guide by SourceryThis pull request addresses a terminology issue by renaming the File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tovrstra - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| assert shells[0].nexp == 6 | ||
| assert shells[1].nexp == 3 | ||
| assert shells[2].nexp == 1 | ||
| assert shells[3].nexp == 2 | ||
| assert shells[4].nexp == 1 |
There was a problem hiding this comment.
suggestion (testing): Consider adding edge cases for nexp values
It would be beneficial to add tests for edge cases such as nexp being 0 or negative, even if these cases are not expected in normal operation. This ensures robustness against unexpected input.
| assert shells[0].nexp == 6 | |
| assert shells[1].nexp == 3 | |
| assert shells[2].nexp == 1 | |
| assert shells[3].nexp == 2 | |
| assert shells[4].nexp == 1 | |
| assert shells[0].nexp == 6 | |
| assert shells[1].nexp == 3 | |
| assert shells[2].nexp == 1 | |
| assert shells[3].nexp == 2 | |
| assert shells[4].nexp == 1 | |
| assert all(shell.nexp >= 0 for shell in shells) |
| assert mol.title == "h_sto3g" | ||
| assert len(mol.obasis.shells) == 1 | ||
| assert mol.obasis.nbasis == 1 | ||
| assert mol.obasis.shells[0].nprim == 3 | ||
| assert mol.obasis.shells[0].nexp == 3 |
There was a problem hiding this comment.
suggestion (testing): Add tests for invalid nexp values
Consider adding tests to handle invalid nexp values, such as non-integer or negative values, to ensure the system behaves correctly under these conditions.
| assert_equal(np.sum([shell.nexp * shell.nbasis for shell in mol.obasis.shells]), 24) | ||
| assert_equal(len(mol.obasis.shells), 6) | ||
| assert_equal(np.sum([shell.nprim for shell in mol.obasis.shells]), 18) | ||
| assert_equal(np.sum([shell.nexp for shell in mol.obasis.shells]), 18) | ||
| assert_equal(mol.charge, 0.0) | ||
| assert_equal(mol.nelec, 9) | ||
| assert_equal(mol.natom, 4) | ||
| assert_equal(mol.energy, -3.90732095e01) | ||
| assert_allclose([shell.angmoms[0] for shell in mol.obasis.shells], [0, 0, 1, 0, 0, 0]) | ||
| assert_allclose([shell.icenter for shell in mol.obasis.shells], [0, 0, 0, 1, 2, 3]) | ||
| assert_allclose([shell.nprim for shell in mol.obasis.shells], [3, 3, 3, 3, 3, 3]) | ||
| assert_allclose([shell.nexp for shell in mol.obasis.shells], [3, 3, 3, 3, 3, 3]) |
There was a problem hiding this comment.
suggestion (testing): Add tests for boundary values of nexp
Consider adding tests for boundary values of nexp, such as 0 or very large numbers, to ensure the system can handle these cases without issues.
| @@ -122,7 +122,7 @@ def test_load_mkl_ethanol(): | |||
| # assert_allclose(mol.obasis.shells[-1].coeffs[-1, -1], 0.181380684) | |||
| assert_equal([shell.icenter for shell in mol.obasis.shells[:5]], [0, 0, 1, 1, 1]) | |||
| assert_equal([shell.angmoms[0] for shell in mol.obasis.shells[:5]], [0, 0, 0, 0, 1]) | |||
| assert_equal([shell.nprim for shell in mol.obasis.shells[:5]], [3, 1, 6, 3, 3]) | |||
| assert_equal([shell.nexp for shell in mol.obasis.shells[:5]], [3, 1, 6, 3, 3]) | |||
There was a problem hiding this comment.
suggestion (testing): Include tests for invalid nexp values
It would be beneficial to include tests for invalid nexp values, such as non-integer or negative values, to ensure the system handles these gracefully.
| assert_equal([shell.nexp for shell in mol.obasis.shells[:5]], [3, 1, 6, 3, 3]) | |
| assert_equal([shell.nexp for shell in mol.obasis.shells[:5]], [3, 1, 6, 3, 3]) | |
| assert_equal(mol.mo.coeffs.shape, (39, 39)) | |
| assert_equal(mol.mo.energies.shape, (39,)) | |
| assert_equal(mol.mo.occs.shape, (39,)) | |
| with pytest.raises(ValueError): | |
| mol.obasis.shells[0].nexp = -1 | |
| with pytest.raises(TypeError): | |
| mol.obasis.shells[0].nexp = "invalid" |
This is an attempt to solve a terminology issue that has caused a lot of confusion in the past. It addresses part of #256. (I'm going through these clarifications before continuing with #191 to make future PRs related to #191 easier to process.)
The term number of primitives, abbreviated now in IOData is
nprim(property of theShellclass), has different interpretations in different context and file formats:Number of primitives per shellis an array with contraction lengths. Because this is one of the earliest supported file formats, it also led to the namenprimin theShellclass.GAUSSIANline contains a number of primitives, e.g.74 PRIMITIVES, which refers to the total number of individual basis functions, e.g. three primitives for a p-type shell (X, Y and Z), etc.Nprimsrefers to the sum of all contraction lengths, counting those of SP generalized contractions twice, andNprimshellis the sum of all contraction lengths, counting those of SP generalized contractions once.I've considered a few alternatives for the name of the
Shell.nprimproperty:contraction_length, too long. This name is also assumes some familiarity with basis set terminology.nexp(as in this PR) refers to its literal meaning, i.e. number of exponents in a contracted shell. (It is also implemented asreturn len(self.exponents)I prefer this one because it is short and semantic. It also uses thensomethingform to refer to the size of an array along one of its dimensions, as found elsewhere in IOData.nexponentis possible, a bit clearer, more typing.I'm trying to make only one name change per PR, to keep things easy to review. In case of concerns about other attribute names, properties, please add them to #256.
nprim-like variables in the WFN, WFX and MWFN formats were not renamed when they referred to the meaning of that file format (and not the one we had in the Shell class.)I will YOLO-merge this on Wednesday, July 10 unless reviewed earlier.
Summary by Sourcery
This pull request addresses a terminology issue by renaming the
nprimproperty tonexpacross various files and test cases to improve clarity and consistency in the codebase.nprimproperty tonexpacross multiple files to improve clarity and consistency in terminology.nprimtonexp.