Skip to content

Refactor Arithmetic Operations on AmplitudeVector - #234

Open
jonasleitner wants to merge 5 commits into
masterfrom
amplitude
Open

Refactor Arithmetic Operations on AmplitudeVector#234
jonasleitner wants to merge 5 commits into
masterfrom
amplitude

Conversation

@jonasleitner

Copy link
Copy Markdown
Contributor

This PR refactors the arithmetic operations implemented on the AmplitudeVector class such that missing blocks are consistently treated as zero blocks.

Previously, this was only the case for __add__, while the other operations employed __forward_to_blocks, which raised an exception for differing blocks. Moreover, dot showed inconsistent behavior, ADC1.dot(ADC2) gave the ph overlap, while ADC2.dot(ADC1) raised an exception due to a missing pphh block. The in-place operators, also employed __forward_to_blocks, which created a new AmplitudeVector instance and thus were not really in-place.

In this PR I explicitly implemented the arithmetic operations ensuring that inputs and result don't share memory (libadcc.Tensors are copied if necessary). Moreover, in-place operations mutate the current instance modifying/deleting existing blocks.
Finally, type hints are introduced throughout AmplitudeVector.py

A Note on the reflective operations:
libadcc.Tensor * Amplitude (same for other operations) is not possible, since the backend raises a TypeError. Thus, __rmul__ etc. are not reachable for this case and the implementation contains in principle dead code.

Comment thread adcc/AmplitudeVector.py Outdated
Comment thread adcc/AmplitudeVector.py
@overload
def dot(
self, other: Sequence["AmplitudeVector"]
) -> np.ndarray[tuple[int], np.dtype[np.float64]]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We would currently mix plain np.ndarray and parameterized np.ndarray[...]. I would prefer the parameterized form where possible. For example, ElectronicTransition.py uses the unparameterized form.

What do you think @jonasleitner ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also prefer the parametrized form including shape and optionally the data type. However, I would suggest to resolve this at a later stage once we revisit the corresponding files regarding type hints?

Comment thread adcc/AmplitudeVector.py Outdated
Comment thread adcc/AmplitudeVector.py Outdated
Co-authored-by: Friederike Schneider <110526409+frieschneider@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants