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

Add tests for density evaluations #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kimt33
Copy link
Collaborator

@kimt33 kimt33 commented Jun 26, 2019

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

There already is a test that compares density evaluation with horton, but the
density matrix used was an identity matrix. Just in case, the density
evaluations are compared for both spherical and cartesian contractions for
random symmetric density matrix.

There were some suspicions that either the density evaluations or the orbital evaluations are giving the wrong results. This PR confirms that the density evaluations are correct (at least in comparison to HORTON).

Type of Changes

Type
🐛 Bug fix

Related Issue

Depends on PR #93.

@codecov
Copy link

codecov bot commented Jun 26, 2019

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (03eff2c) to head (e04378d).
Report is 172 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #94   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files          28       28           
  Lines        1408     1408           
  Branches      324      324           
=======================================
  Hits         1403     1403           
  Misses          4        4           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kimt33 kimt33 changed the title Add tests for density evalutaions Add tests for density evaluations Jun 26, 2019
There already is a test that compares density evaluation with horton, but the
density matrix used was an identity matrix. Just in case, the density
evaluations are compared for both spherical and cartesian contractions for
random symmetric density matrix.
@FarnazH
Copy link
Member

FarnazH commented Jan 10, 2024

@maximilianvz I just invited you to gbasis team until you accept it. I won't be able to assign you as a reviewer. Please review this PR, after PR #140 is merged. Please commit any changes required (instead of adding comments) that you believe are needed to approve this PR. Thanks.

@maximilianvz
Copy link
Collaborator

maximilianvz commented Jan 11, 2024

@FarnazH, I have taken a look at this PR, and while there are obvious, easy changes to be made regarding coord_types as per PR #140, there seems to be a problem with assertions here:

In the state of the repo at the time that this PR was made, the assertions pass fine. However, in the repo's current state, evaluate_density outputs zeros in its return that weren't present at the time of this PR, causing the assertion to fail. For example, in this assertion (after making appropriate API changes), at indices [ 18, 22, 23, 35, 40, 41, 42, 51, 52, 53, 60, 65, 66, 75, 76, 77, 80, 82, 100, 101, 105, 106, 107, 110], evaluate_density(density_matrix, cartesian_basis, grid_3d) (non-highlighted array) and horton_density (highlighted array) have the following, differing values:

Screenshot 2024-01-11 at 5 29 09 PM

I'm guessing that, at some point in the 4.5 years since this PR was created, something about evaluate_density changed such that this assertion no longer passes. Why this is the case is beyond me, so I encourage others to look into it.

@leila-pujal
Copy link
Collaborator

I looked into this and, if I checked this correctly, this test fails because the correct output has negative values and we are making those zero now (see

return evaluate_density_using_evaluated_orbs(one_density_matrix, orb_eval).clip(min=0.0)
). You can see in @maximilianvz screenshoot that all those values are negative.
Based on the PR, looks like the reference data comes from Horton, using a "random symmetric density matrix". I am not sure what this random means here, if it is not physically relevant, meaning if a "not random" symmetric density matrix is used we are not going to have this large negative values. From what I know density can not be negative and that is why we were filtering the output for those numbers. @FarnazH, @PaulWAyers any insights into this would be good to see if we should keep this test or not.

@PaulWAyers
Copy link
Member

I think the test should use a DM from a data file. We should also only test positive density values, though we should test that the correct behavior is observed for negative density values too.

The other choice is to build an ANO-centric example along the lines I recommended elsewhere.

Comment on lines +317 to +318
horton_density = np.load(find_datafile("data_horton_hhe_cart_density_rand.npy"))
density_matrix = np.load(find_datafile("data_horton_hhe_cart_dm_rand.npy"))
Copy link
Member

@FarnazH FarnazH Jan 12, 2024

Choose a reason for hiding this comment

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

This tests against HORTON (based on these test files included in the repo), so I don't understand why a random density matrix is used. @maximilianvz, can you update this test and its files to use a real example?

@PaulWAyers
Copy link
Member

@marco-2023 there is an $\ce{HHe}$ test that is referred to here. It may be what you are looking for so you may be able to use the same example, but I would do $\ce{HHe+}$, as $\ce{HHe}$ is sort of boring (and is harder, besides).

@FarnazH FarnazH self-assigned this Jun 22, 2024
@FarnazH FarnazH added the low priority Low priority issue label Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low priority Low priority issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants