Closed maxscheurer closed 3 years ago
This pull request introduces 10 alerts when merging f7456a2ed886abfe66caf3a546787594b6b2dd09 into 60e201cccaa684579dca0ae7eb28232d3adce482 - view on LGTM.com
new alerts:
This pull request introduces 10 alerts when merging 0373e318e5ecb78479db5aa83a85f6ebb8c42f4a into 55436b4bbf6c8a716c99add3f15adde341acb9ff - view on LGTM.com
new alerts:
This pull request introduces 10 alerts when merging d851134db27dabe90da57b03240713d5b4c36b83 into 55436b4bbf6c8a716c99add3f15adde341acb9ff - view on LGTM.com
new alerts:
There is a bug in the testdata generator where from and to are flipped over, see
https://github.com/adc-connect/adcc-testdata/blob/master/adcctestdata/dump_reference.py#L206
but even taking that into account into the tests only ADC(0) and ADC(1) work. Could you have made a small mistake at ADC(2) level, which magically compensates in the dipoles?
This pull request introduces 1 alert when merging 7bb97809d57034151cbba06c68ca4098594103f1 into 55436b4bbf6c8a716c99add3f15adde341acb9ff - view on LGTM.com
new alerts:
There is a bug in the testdata generator where from and to are flipped over, see
I just spent some time checking this and I am no longer convinced this is a bug by comparing with the adcman output. Before making a call on this issue we have to investigate more thoroughly.
This pull request introduces 1 alert when merging 4aab16122df81096a30abb879e3777990de10680 into d478e95f7bb9c42d78bffe0faded5b90164214f4 - view on LGTM.com
new alerts:
I think you're right. I've made some changes to have a more consistent interface... but I have no idea why the densities are still different. Transition moments seem to be okay...
I think we need a decision on how to proceed with this PR. The question is if we want to hunt down what causes the discrepancies in the densities (I currently don't have an idea why this could be, so I don't know where to start debugging). It's strange that already adc0
/adc1
densities are not in agreement with reference data.
Ok so one thing is clear. We can't merge it before we don't understand the discrepancies.
So let's start over from the beginning step by step. First we make a list. Let's pick the water sto-3g (using the geometry in the adcc repo) as the simplest test case and just manually compare one more time:
If something comes to your mind, just add it. If you have done one thing, tick it off. I'll try to do the second thing tonight and then we see how to continue.
Ah sorry that was wrong thinking, because the dipole moments actually agree, right? It's only the densities which differ?
adcman sometimes does weird things where the order of from state and to state are weirdly mixed (so first to
, then from
or something like that). Could it be that we differ in that aspect? Maybe our equations are just implemented in reverse?
Note: That might not be an error, that might just cause matrices to be transposed or something like that. But just some idea to check.
Yes, transition dipole moments are correct, but the densities are not in agreement.
They are indeed just transposed... 🤦
Yeah ok ... but why? If we understand that we are good to go.
And of course ... is our version consistent and do we understand what it means. (then of course we should document it that we don't forget)
I'm investigating...
I think the testdata from/to are reversed as you said previously here. The key format is definitely from-to
according to my investigation.
BUT the loops in adcman are a bit weird, so we get (dummy code):
for i in range(nstates):
for j in range(i):
# compute density i->j and store it
🤯 🤯 🤯
whereas we compute (the reasonable way)
i = bla
for j in range(i + 1, nstates):
# compute density i->j
The key format is definitely from-to according to my investigation.
Hmm. Let's discuss. So my argument is: If you have a transition x -> y
then the transition dipole moment is tr (y* μ x)
. Now representing both states in a finite AO basis C
(i.e projecting into the space spanned by C
) gives the approximation tr[y* C C* μ C C* x] = tr[M C* x (C* y)*]
, where M
are the dipole moment integrals in AO. Denoting now X = C*x
and similarly Y
the representation of the states y
and x
in the same AO basis yields tr(M X Y*)
, i.e. the transition density matrix X Y*
. In other words the final state should be the one which gets an adjoint (no surprise actually if you look at the transition dipole integral <y | μ x>
).
In your implementation amplitude_r
gets the adjoint (see e.g. s2s_tdm_adc0
), therefore it is the final state. In other words
is the right way round and our transition density matrices should be correct. Would you agree to that reasoning? What does @Drrehn think? Am I thinking straight here?
I have to check the Adcman paper again... the implementation should be in agreement with the published equations. So, if we can all agree on the way it is done in the Adcman paper, then I'm fine with that.☺️
After a long and painful struggle with these transition densities, it's looking really good to me now. 🎉
OK. I'll try to take a look tonight.
Ah and another small thing: Could you add something to our docs about State2States
. What I have in mind is maybe the way to plot a spectrum for spin-flip to the tutorial and one of the spin-flip examples could be good.
I've addressed the reviews and added a small paragraph to the docs.
Good idea.
Should we take the current version? 😄
apparently ...
I thought that was clear ... sorry.
Python-side equations for State-to-state transition densities.
[ ] CVS-ADC (I'm not really in the mood to add this though 😄)➡️ postponeExcitedStates
/Excitation
(this is the hardest part) (preliminary version working)