Pariser parr #129
Pariser parr #129
Conversation
moha/rauk/PariserParr.py
Outdated
| atom1_name, site1 = get_atom_type(atom1) | ||
| atom2_name, site2 = get_atom_type(atom2) | ||
|
|
||
| if site1 < num_sites and site2 < num_sites: |
There was a problem hiding this comment.
I'm not sure if we need this check. Can't we have value of the site to be equal to number of sites? Let's say site 6 for the total of 6 sites?
|
The gh actions are failing because there is no file |
|
Can you specify where the data came from? Some of the values are not quite what I would have expected, but perhaps due to the source. Negative electron affinities are OK here, probably, I guess....it's a matter of taste as to what to do with negative electron affinities. |
I took this data from https://en.wikipedia.org/wiki/Electron_affinity_(data_page). I am not sure about the truthfulness of the data... |
|
That's fine. With that documented (via pull request and eventual package documentation) we are in good shape. We can make an executive decision on negative electron affinities later. Realistically, they mostly occur for elements that are not very interesting for PPP theory. |
Nice! I've already drafted some documentation that explains this: https://github.com/theochem/ModelHamiltonian/blob/PariserParr/examples/rauk.ipynb |
|
@PaulWAyers do you have any other resource in mind that we can use for electron affinities? |
|
Not really. This is good enough for this purpose. We could use |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Great job! I especially like the last test when we test the energy spectrum of hamiltonian. However, there are some comments regarding the consistency of docstrings and parameter order. Also, some tests that I wouldn't expect to work are working. I left more detailed explanations in the in-line comments
moha/hamiltonians.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| connectivity_ppp: Union[list, np.ndarray] |
There was a problem hiding this comment.
we need to make sure that docstring consistent with the parameters. For example, there is no parameter connectivity_ppp in the input parameters.
moha/hamiltonians.py
Outdated
| List of tuples specifying sites and bonds | ||
| np.ndarray of shape (n_sites, n_sites) | ||
| between sites for the PPP model. | ||
| connectivity_heisenberg: np.ndarray |
There was a problem hiding this comment.
We need to make the docstring consistent with the the input parameters
| scipy.sparse.csr_matrix or np.ndarray | ||
| """ | ||
| two_body_ppp = self.ocupation_part.generate_two_body_integral( | ||
| basis, dense, sym) |
There was a problem hiding this comment.
It looks like order of parameters for generating two body hamiltonian for PPP and Heisenberg hamiltonians are inconsistent. We need to change order of Heisenberg model parameters and then change it in this line
moha/test/test_tjuv.py
Outdated
|
|
||
| heisenberg = HamHeisenberg(J_eq=J_eq, J_ax=J_ax, mu=0, connectivity=connectivity) | ||
| heisenberg_zero = heisenberg.generate_zero_body_integral() | ||
| hpp = HamPPP(connectivity=connectivity, alpha=alpha, beta=beta, gamma=np.zeros((2, 2)), charges=None, sym=None, u_onsite=[1, 1]) |
There was a problem hiding this comment.
This test feels a bit fishy to me. The connectivity implies 6 site model. However, we provide gamma for 2 site system, and the same happening with u_onsite. I would expect this test to break. We need to investigate what's going on here
There was a problem hiding this comment.
I think this breaks! I have committed the changes.
moha/test/test_tjuv.py
Outdated
| heisenberg = HamHeisenberg(J_eq=J_eq, J_ax=J_ax, mu=0, connectivity=connectivity) | ||
| heisenberg_one_body = heisenberg.generate_one_body_integral(basis='spatial basis', dense=True) | ||
|
|
||
| hpp = HamPPP(connectivity=connectivity, alpha=alpha, beta=beta, gamma=np.zeros((2, 2)), charges=None, sym=None, u_onsite=u_onsite) |
There was a problem hiding this comment.
Another test, in which gamma is inconsistent with number of sites.
There was a problem hiding this comment.
Maybe it's because we are not checking if the size of gamma is correct in the two body term functions
Fixing CI errors.