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

Move MTMs, diffdms and transition dms to python #77

Merged
merged 4 commits into from
Jun 17, 2020
Merged

Move MTMs, diffdms and transition dms to python #77

merged 4 commits into from
Jun 17, 2020

Conversation

mfherbst
Copy link
Member

@mfherbst mfherbst commented Jun 16, 2020

  • Update adccore to the version including the missing patches
  • diffdm
  • transition dms
  • MTMs
  • Getattr in module file
  • as b more locally.
  • Check timings against master.

@mfherbst
Copy link
Member Author

@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:

f2 = (1 / sqrt(2)) * einsum("Ik,kjab->jIab", dipop[b.co], mp.t2(b.oovv))

Thoughts?

@mfherbst
Copy link
Member Author

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.

@mfherbst
Copy link
Member Author

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?

@mfherbst mfherbst added adccore Related to adccore library and interface to it properties Related to property calculations and ISR question Further information is requested labels Jun 16, 2020
@mfherbst mfherbst marked this pull request as ready for review June 16, 2020 18:52
@maxscheurer
Copy link
Member

I‘m giving this a thorough look first thing tomorrow morning!

@maxscheurer
Copy link
Member

maxscheurer commented Jun 17, 2020

  • 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?

Copy link
Member

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

This looks really good already! Just some minor comments...

adcc/block.py Show resolved Hide resolved
adcc/kernel/modified_transition_moments.py Outdated Show resolved Hide resolved
adcc/kernel/modified_transition_moments.py Outdated Show resolved Hide resolved
adcc/kernel/state_diffdm.py Outdated Show resolved Hide resolved
adcc/kernel/test_modified_transition_moments.py Outdated Show resolved Hide resolved
extension/export_AdcMatrix.cc Outdated Show resolved Hide resolved
extension/export_OneParticleOperator.cc Outdated Show resolved Hide resolved
@Drrehn
Copy link
Contributor

Drrehn commented Jun 17, 2020

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

@mfherbst
Copy link
Member Author

@maxscheurer Thanks for the comments.

  • folder name: I see your point. But keep in mind if we later distribute adccore separately as a conda / pip package than the name of the module will likely change to adccore as well for consistency. I never liked libadcc ... it was just a hack because adccore.so was already the statically compiled part, but if this has the python bindings included, there is no reason for libadcc. Of course we could also think of a different name for that library ... then I don't mind using core here.
  • prefix: No you don't misunderstand and I see your point, but the idea is that the user will always use the dispatch function and never call into the specific methods. Unless of course he is doing low-level hacking and then using the full thing like 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.
  • 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.

@Drrehn https://guides.github.com/features/mastering-markdown/

@Drrehn
Copy link
Contributor

Drrehn commented Jun 17, 2020

  • 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

@mfherbst
Copy link
Member Author

Yes absolutely! That makes sense and that I buy.

Copy link
Member

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

LGTM! Nice!
If the timings are alright, feel free to merge this! 🚀

@mfherbst mfherbst merged commit 8268335 into master Jun 17, 2020
@mfherbst mfherbst deleted the pyprop branch June 17, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adccore Related to adccore library and interface to it properties Related to property calculations and ISR question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants