search

adc-connect / adcc

adcc: Seamlessly connect your program to ADC
https://adc-connect.org
GNU General Public License v3.0
32 stars 19 forks source link

AdcMatrix for different ADC schemes #129

Open Drvanon opened 3 years ago

Drvanon commented 3 years ago

Currently this library is favoring the adc_pp a bit: the AdcMatrix class as well as the AdcAmplitudeVector and some others have attributes like ph and pphh or block_ph_ph_1, that are unique to the PP scheme. This makes it harder than necessary to extend the class for different schemes. At this point in time I am subclassing from AdcMatrix in my fork, but that means there are properties on my class that represent no physical thing.

My suggestion would be to use singles and doubles as properties (and use a format like block_singles_doubles_0(hf, mp, intermediates)) and let those be subclassed for different schemes, because this terminology overlaps for all ADC schemes. As far as I am aware, the workflow would not even need major overhaul to support multiple schemes.

I saw some remainders of a notation like this, but you have seemed to have stepped away from this, so it might also be the case that I am missing something that you found through the development process.

maxscheurer commented 3 years ago

AmplitudeVector

This is just a dict, you can name the attributes/keys anything you want. AmplitudeVector(blabla=tensor2, bla=tensor3) is valid. Hence, no point creating a DipAmplitudeVector in your fork. In case you got confused by the name blocks_ph, this will be removed/renamed anyways in v0.16.0.

that are unique to the PP scheme

Since the folder is called adc_pp, what else do you expect?!

Indeed, we found the terminology singles and doubles bad, that's why we stepped away from it.

I think most of what you want to achieve can be done by creating a clever AdcMatrix class and that's it. Probably some things could be moved around a little bit here and there, but I don't see the point "overhauling" major parts of the code. What do you think, @mfherbst?

mfherbst commented 3 years ago

Thanks @Drvanon for getting in touch and thanks for extending adcc ;).

I pretty much agree with @maxscheurer. Since we have not really looked into other ADC schemes a lot, there might be some rough edges at places that make some refactoring necessary, but overall I'd be surprised if an overhaul was really required as the general workflow should stay the same.

Drvanon commented 3 years ago

Thank you for all of your quick replies! I see what you mean with the AmplitudeVector class.

@maxscheurer, I was trying to address the attributes/variables in the AdcMatrix class, particularly the default_blocks class variable, which currently is designed for PP only, as well as the sanity check and the matrix block function that is called (see my commits to https://github.com/Drvanon/adcc/blob/master/adcc/AdcMatrix.py).