Closed mfherbst closed 4 years ago
@Drrehn @maxscheurer Now that I implemented all MTM equations in the same python file I an wondering whether the CVS f2
contribution needs an antisymmetrise
as well. Right here: https://github.com/adc-connect/adcc/blob/b10592e5dfeca87b8242b5eb73891ea3d27f7c32/adcc/kernel/modified_transition_moments.py#L73
Thoughts?
Also we should bikeshed the name of the folder. From my end pro kernel
is that it is an unused name in adcc (core is already connected to the C++ library). Cons is that it mathematically is something different. Both terms are also pretty much meaningless. Same as equations, which is too generic (and probably this folder will not be the only place with equations and this folder will not just be equations).
I am tempted to leave it as kernel
even though I don't like it. I am open for other suggestions.
Also one thing we could do is not have so long names inside the kernel files. Since the files are already named like modified_transition_moments
I think it's fine to name the functions just after the ADC method and drop the prefix. Thoughts?
I‘m giving this a thorough look first thing tomorrow morning!
CVS f2
: @Drrehn, please correct me if I'm wrong: IIRC the antisymmetrise
is not needed because the symmetry only arises from the ab
indices of the t2
, which are retained during contraction.
folder name: I'm against kernel
and for core
. The association with the adccore repo is just manifested by the name of the repo, not really in the Python code (libadcc
). TBH I don't like either option...
I think it's fine to name the functions just after the ADC method and drop the prefix
Can you be more specific about this? I expect there to be clashes with duplicate names when the actual functions are being imported somewhere, i.e.,
from adcc.modified_transition_moments import adc2
from adcc.state_densities import adc2
Probably I misunderstand?
I agree with @maxscheurer that the antisymmetrise is not needed becaused the indices are not touched even though I don't know how to make my reply as fancy with bullet points
@maxscheurer Thanks for the comments.
modified_transition_moments.adc2
is ok, I think. My point is that shorter names in the modified_transition_moments.py
lead to shorter and simpler to digest code and thus allows you to better get an overview of the equations, which is what really matters here. and not the name. Also the use case of from ... import adc2
is extremely rare.@Drrehn https://guides.github.com/features/mastering-markdown/
- antisymmetrise: Ok then we leave it. It's just that for the amplitude vectors it is antisymmetrised and to me contracting MTMs with dips and amplitudes with TDMs was always kind of pendants, so I would expect by symmetry an antisymmetrisation to be needed in the MTMs.
I guess the idea is that they are already antisymmetrised in a and b from the t2-amplitude and i and j have no symmetry because of core and valence
Yes absolutely! That makes sense and that I buy.