-
Notifications
You must be signed in to change notification settings - Fork 21
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
Additional operator integrals and selection of gauge origin #169
base: master
Are you sure you want to change the base?
Conversation
…hecks for gauge origin dependent operators. Change the default selection of gauge origin for operator integrals to origin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! Some initial comments from a first quick look 😁
Can you please have a look at this, too, @apapapostolou @Drrehn?
adcc/backends/pyscf.py
Outdated
def get_coordinates(self): | ||
coords = self.scfres.mol.atom_coords() | ||
coords = np.reshape(coords, (3 * coords.shape[0])) | ||
return coords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do the coordinates/other properties above need to be exposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to be able to compute nuclear momets (electric quadrupole & electric quadrupole traceless) with different gauge origins in responsefun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, a selection of different gauge origins should be possible within the get_nuclear_multipole
method of the HartreeFockProvider
class. However, this is a little more complicated here than for the operator integrals because also libadcc
would need to be changed for this and we only need it for multipoles with an order > 1. Then the coordinates etc. would not need to be exposed, which would definitely make more sense. @maxscheurer do you have an idea how we could handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new properties can then be deleted.
… into tests_operators
The selection of the origin is now an argument of the respective property, i.e. state.reference_state.operators.magnetic_dipole('mass_center'). I changed the tests so that I can reproduce some reference data I previously generated. However, I think my tests and how I generated this reference data can still be improved. Thus, @apapapostolou , @Drrehn , @maxscheurer maybe we should talk about how I should generate reference data and how to change the tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the reference data, I think you should not regenerate all the reference data for each gauge origin but only have different operator integrals.
adcc/backends/pyscf.py
Outdated
def get_coordinates(self): | ||
coords = self.scfres.mol.atom_coords() | ||
coords = np.reshape(coords, (3 * coords.shape[0])) | ||
return coords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, a selection of different gauge origins should be possible within the get_nuclear_multipole
method of the HartreeFockProvider
class. However, this is a little more complicated here than for the operator integrals because also libadcc
would need to be changed for this and we only need it for multipoles with an order > 1. Then the coordinates etc. would not need to be exposed, which would definitely make more sense. @maxscheurer do you have an idea how we could handle this?
Concerning the new tests:I updated the test and in order to do that I also needed to generate new hf data with pyscf. You can find the modified file in my HeiBox: I needed to regenerate new adcc reference data, I changed the file The new reference files can be generated with the provided scripts. Should I add the updated reference data to the HeiBox as well? |
Probably the reference data (adcc ref, that is) need to be re-generated once #175 is merged because that affects the data as well. |
@frieschneider #175 has been merged. You can now regenerate the reference data and then make it available to me so that I can upload it. |
adcc/backends/pyscf.py
Outdated
def get_coordinates(self): | ||
coords = self.scfres.mol.atom_coords() | ||
coords = np.reshape(coords, (3 * coords.shape[0])) | ||
return coords |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new properties can then be deleted.
///@{ | ||
/** Fill a buffer with nuclear multipole data for the nuclear multipole of | ||
* given order. */ | ||
virtual void nuclear_multipole(size_t order, scalar_type* buffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would definitely not delete this from the base class because nuclear dipoles should be a requirement for the backend. I would rather distinguish between nulcear_dipole
and nuclear_multipole
and the latter could then be implemented on the Python side only.
6230903
to
a77f3bc
Compare
|
@frieschneider I uploaded the reference data to https://wwwagdreuw.iwr.uni-heidelberg.de/adcc_test_data/0.7.0/ for you. You need to change the URL accordingly and also adapt the |
768cf35
to
22c8d13
Compare
Operator integrals:
For the pyscf backend I added additional operator integrals:
and the
as reference state properties.
The magnetic dipole moment operator integrals are zero for adc1 for linear molecules for the coordinate parallel to the main axis. I added to the conjugate solver to return "is zero" for this integral and exit.
Testing:
test_properties_consistencty.py:
I added the different gauge origins, which would require the addition of testdata.
test_properties.py
I added the electric quadrupole, requires regenerating testdata and new testdata.
ToDo:
Workflow:
Selection of the gauge origin:
state = adcc.adc1(scfres, n_states=1, gauge_origin="origin")