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

Matrix/Tensor product between two PauliWords now returns PauliSentence #5054

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

Qottmann
Copy link
Contributor

@Qottmann Qottmann commented Jan 12, 2024

Following discussion here](#5017 (comment)) changing the default behavior of products between two PauliWord instances.

old (see comment in discussion)
One thing I am still unsure about at the moment is for pure tensor products (where no coefficient is raised). E.g.

pw1 = PauliWord({0:"X"})
pw2 = PauliWord({1:"X"})

I would find it counterintuitive to get a PauliSentence from pw1 @ pw2 but would expect PauliWord({0:"X", 1:"X"})

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3ab3a32) 99.68% compared to head (d04267c) 99.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5054      +/-   ##
==========================================
- Coverage   99.68%   99.67%   -0.02%     
==========================================
  Files         394      394              
  Lines       35730    35465     -265     
==========================================
- Hits        35619    35351     -268     
- Misses        111      114       +3     

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

@Qottmann Qottmann changed the title [WIP] Matrix/Tensor product between two PauliWords now returns PauliSentence Matrix/Tensor product between two PauliWords now returns PauliSentence Jan 15, 2024
@Qottmann
Copy link
Contributor Author

See no significant performance regression in this (admittably) simple test benchmark

image

def foo(n):
    sentences = [
        PauliSentence({PauliWord({i:"X", i+1:"X"}): 1.23, PauliWord({i:"Y", i+1:"Z"}): 5.})
        for i in range(n)
    ]
    res = sentences[0]
    for ps in sentences:
        res @= ps

ns = range(2, 10, 2)
times = [_timeit(foo, n, reps=10) for n in ns]

Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

Existing changes look good to me so far, but it will be missing some docs updates once the Pauli4 PR is merged in.

Looks to me like the examples in the docstring for PauliWord will need an update for this change, as well as the qml_pauli.rst file. I guess the exact form of the change might depend on the decision about a deprecation cycle discussed above (and certainly for the Pauli4 PR to merge).

@lillian542
Copy link
Contributor

I would find it counterintuitive to get a PauliSentence from pw1 @ pw2 but would expect PauliWord({0:"X", 1:"X"})

I think it's fine - you also get a PauliSentence from PauliSentence({pw1: 2}) - pw1 that's just the PauliWord with a coefficient of 1.

I can see it either way. I think it's probably better to have the result be a consistent type instead of simplifying in certain cases and ending up with different possible outcomes (in terms of the type of object). But I wouldn't be opposed to just returning a PauliWord either.

@Qottmann
Copy link
Contributor Author

I can see it either way. I think it's probably better to have the result be a consistent type instead of simplifying in certain cases and ending up with different possible outcomes (in terms of the type of object)

I agree with that, the (marginal) performance increases dont justify that special case. Overall better to have something that just works for a user than something that is complicated to handle and slightly faster here.

Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

Thanks @Qottmann, looks good to me! 🎉

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 suggesting this.

I think the only way to really go about this is with a hard breaking change, but it's one that shouldn't particularily influence anyone, and I can see everyone being happy with.

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @Qottmann!

@Qottmann Qottmann merged commit 8a43135 into master Jan 19, 2024
35 checks passed
@Qottmann Qottmann deleted the pwmult branch January 19, 2024 16:03
mudit2812 pushed a commit that referenced this pull request Jan 19, 2024
#5054)

Following discussion
here](#5017 (comment))
changing the default behavior of products between two `PauliWord`
instances.


old (see comment in discussion)
~~One thing I am still unsure about at the moment is for pure tensor
products (where no coefficient is raised). E.g.~~
```
pw1 = PauliWord({0:"X"})
pw2 = PauliWord({1:"X"})
```
~~I would find it counterintuitive to get a PauliSentence from `pw1
@ pw2` but would expect `PauliWord({0:"X", 1:"X"})`~~
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.

4 participants