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

Add __iadd__ method to PauliSentence #4662

Merged
merged 14 commits into from
Oct 30, 2023
Merged

Add __iadd__ method to PauliSentence #4662

merged 14 commits into from
Oct 30, 2023

Conversation

AnuravModak
Copy link
Contributor

So here is the first draft of the changes that i have done: towards #4658

So for the task of defining def __ iadd__(self, other):

  1. The iadd method takes two arguments, self and other.

  2. It checks which of the two sentences (self or other) is smaller in terms of the number of terms.

  3. It iterates over the smaller sentence, adding its terms and coefficients to the larger sentence in-place, without using the copy.

  4. The method returns self to indicate in-place modification.

And for testing part, I have tried to optimize the code as far as possible, and from my understanding it should cover all the conventional test cases, please let me know if there is any loop hole or if something is not wrong.

@albi3ro
Copy link
Contributor

albi3ro commented Oct 10, 2023

Thanks for opening this PR @AnuravModak . Just realized a potential clarification. If __iadd__ is called, we should only add the terms to self and leave other unchanged. It will be up to the user to add terms to the bigger object.

For example if we have:

a = [1]
b = [2, 3]
a += b

b is left unchanged, while a is modified.

@AnuravModak
Copy link
Contributor Author

so here i have made another PR. The idea was to only change the self and the other remains unchanged and the same has been reflected in def test_iadd(self, string1, string2, result). Please review it and let me know if there is anything else to do.

@trbromley trbromley requested a review from albi3ro October 16, 2023 17:16
@albi3ro
Copy link
Contributor

albi3ro commented Oct 17, 2023

Suppose we have two pauli sentences we want to sum together:

pw1 = PauliWord({0:"X", 1:"Y", 2:"Z"})
pw2 = PauliWord({1:"Y"})
pw3 = PauliWord({0:"Z"})

ps1 = PauliSentence({pw1: 1.0, pw2: 1.0})
ps2 = PauliSentence({pw1: 1.0, pw2: 1.0, pw3: 1.0})

We can use the normal addition:

>>> ps1 + ps2
2.0 * X(0) @ Y(1) @ Z(2)
+ 2.0 * Y(1)
+ 1.0 * Z(0)

Or the in-place addition on the larger sentence:

>>> c_ps2 = copy.copy(ps2)
>>> c_ps2 += ps1
>>> c_ps2
2.0 * X(0) @ Y(1) @ Z(2)
+ 2.0 * Y(1)
+ 1.0 * Z(0)

And get the same answer, as expected.

But what happens if we try and add the larger sentence onto the smaller one right now?

>>> c_ps1 = copy.copy(ps1)
>>> c_ps1 += ps2
>>> c_ps1
2.0 * X(0) @ Y(1) @ Z(2)
+ 2.0 * Y(1)

We end up getting ps1 + ps1, not ps1 + ps2.

What does this behaviour tell us about what is going on? When we are adding a bigger pauli sentence to a smaller one, we are adding self to the object, instead of other. To get the correct answer, we always need to add other, regardless of size.

Does that make sense?

@AnuravModak
Copy link
Contributor Author

AnuravModak commented Oct 17, 2023

Thanks for your clarification, the behavior you want to achieve, as explained in the provided text, is to always add the other (string2 in your test) to the self (string1 in your test) when performing in-place addition using +=. Currently, my iadd method is designed to add the smaller of the two objects to the larger one, which can lead to the undesired behavior you mentioned.

So I am adding the draft code so please confirm if u are looking for something like this:

def iadd(self, other):
    for key in other:
        if key in self:
            self[key] += other[key]
        else:
            self[key] = other[key]
    return self

@albi3ro
Copy link
Contributor

albi3ro commented Oct 17, 2023

Thanks for your clarification, the behavior you want to achieve, as explained in the provided text, is to always add the other (string2 in your test) to the self (string1 in your test) when performing in-place addition using +=. Currently, my iadd method is designed to add the smaller of the two objects to the larger one, which can lead to the undesired behavior you mentioned.

So I am adding the draft code so please confirm if u are looking for something like this:

def iadd(self, other):
for key in other:
if key in self:
self[key] += other[key]
else:
self[key] = other[key]
return self

Exactly 👍

When we had to copy one of the two objects, we wanted to copy the smaller of the two. But we don't have that flexibility of choice with__iadd__. It's on the user to decide which one gets changed instead.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Once we get the test passing (pending copies in the right place so we don't mutate the test inputs), we will also need to add a changelog entry.

This is located at doc/releases/changelog-dev.md. You should add an entry under Improvements and add your name to the list of contributors at the bottom.

Note that we will be creating our release candidate today, so the changelog-dev.md on master will be getting changed in a few hours. So it might be best to wait a few hours to add a changelog entry to avoid merge conflict issues.

Co-authored-by: Christina Lee <[email protected]>
Co-authored-by: Christina Lee <[email protected]>
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (02f982e) 99.64% compared to head (d8d5ab7) 99.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4662      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files         380      380              
  Lines       34266    34012     -254     
==========================================
- Hits        34145    33890     -255     
- Misses        121      122       +1     
Files Coverage Δ
pennylane/pauli/pauli_arithmetic.py 100.00% <100.00%> (ø)

... and 42 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnuravModak
Copy link
Contributor Author

Hey @albi3ro, the Formatting check / black (pull_request) is still failing, is there anything to do from my side?

Co-authored-by: Christina Lee <[email protected]>
Co-authored-by: Christina Lee <[email protected]>
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Thanks for your work and patience @AnuravModak 🎉

I'm happy to approve, and am tagging the team for the second review, but that should go quickly at this point.

@albi3ro albi3ro requested a review from a team October 30, 2023 17:02
@albi3ro albi3ro requested a review from Jaybsoni October 30, 2023 17:02
Copy link
Contributor

@timmysilv timmysilv left a comment

Choose a reason for hiding this comment

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

looks good to me! 🎉

@albi3ro albi3ro enabled auto-merge (squash) October 30, 2023 19:24
@albi3ro albi3ro merged commit 28d255a into PennyLaneAI:master Oct 30, 2023
32 checks passed
@Alex-Preciado Alex-Preciado added the hacktoberfest-accepted Hacktoberfest not merged but in a good shape label Oct 30, 2023
@Alex-Preciado Alex-Preciado linked an issue Oct 31, 2023 that may be closed by this pull request
@trbromley
Copy link
Contributor

Thanks @AnuravModak for your contribution! 🏆

This should be released as part of v0.34 in early January. If you would like us to tag you as part of our Twitter/X marketing, feel free to optionally share your handle here.

@AnuravModak
Copy link
Contributor Author

Thanks @AnuravModak for your contribution! 🏆

This should be released as part of v0.34 in early January. If you would like us to tag you as part of our Twitter/X marketing, feel free to optionally share your handle here.

You can find me on twitter using this handle: @aakhri_modak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest not merged but in a good shape
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add __iadd__ method to PauliSentence
5 participants