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 SProd.terms() method #5287

Merged
merged 8 commits into from
Mar 1, 2024
Merged

Add SProd.terms() method #5287

merged 8 commits into from
Mar 1, 2024

Conversation

Qottmann
Copy link
Contributor

@Qottmann Qottmann commented Feb 29, 2024

Missed to add SProd.terms(), SProd.ops and SProd.coeffs, fixing this here

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@Qottmann Qottmann added this to the v0.35 milestone Feb 29, 2024
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (v0.35.0-rc0@6138d5e). Click here to learn what that means.

Additional details and impacted files
@@              Coverage Diff               @@
##             v0.35.0-rc0    #5287   +/-   ##
==============================================
  Coverage               ?   99.64%           
==============================================
  Files                  ?      399           
  Lines                  ?    36635           
  Branches               ?        0           
==============================================
  Hits                   ?    36506           
  Misses                 ?      129           
  Partials               ?        0           

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

Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni left a comment

Choose a reason for hiding this comment

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

Great. Thank you for adding this!

@Qottmann Qottmann merged commit 0b65514 into v0.35.0-rc0 Mar 1, 2024
39 checks passed
@Qottmann Qottmann deleted the sprodterms branch March 1, 2024 14:21
lillian542 added a commit that referenced this pull request Mar 1, 2024
@lillian542 lillian542 mentioned this pull request Mar 1, 2024
trbromley pushed a commit that referenced this pull request Mar 1, 2024
Merging PR #5287 to add `terms` to `SProd` created problems for Catalyst
that won't be resolved before the release. Here the commit pushing it to
the RC branch is reverted, and we will return to the change next week.
@dime10
Copy link
Contributor

dime10 commented Mar 6, 2024

@Qottmann could you elaborate what the intention here is? Is it that SProd should recursively determine what the terms and coeffs are? Only one level deep or several? Only for specific observables or all?

I believe previously it always returned a single coeff (the scalar) and a single observable (whether compound or not), but happy to be corrected here.

@Qottmann
Copy link
Contributor Author

Qottmann commented Mar 6, 2024

In Prod and Sum the terms() method flattens out the operator to its coefficients and "pure" and simplified ops (i.e. products are executed). The request was for that to be consistens also with SProd such that you always receive a flattened down list of coeffs and ops.

E.g. if you have some operator op = qml.sum(*[qml.s_prod(0.5, X(i)) for i in range(3)]). The terms are [0.5, 0.5, 0.5] and [X(i) for i in range(3)]. Consider that you then later decide to renormalize it by multiplying with a scalar op = 2 * op, but now the terms would just yield [2], [<original_op]`. The motivation was for this to be consistent.

Frankly its not super urgent and probably a matter of taste what one prefers.

more context here on slack

@dime10
Copy link
Contributor

dime10 commented Mar 7, 2024

Thanks @Qottmann! The change is not a problem for us as long as we're aware of the new behaviour, so feel free to re-merge this whenever convenient :)

@dime10
Copy link
Contributor

dime10 commented May 1, 2024

Was this added back or is that not planed anymore?

@albi3ro
Copy link
Contributor

albi3ro commented May 1, 2024

@dime10 Still definitely wanted, but I guess is got backlogged. Probably too late for the next release, but we should definitely look into making this change together in tandem.

@dime10
Copy link
Contributor

dime10 commented Jun 24, 2024

Just checking in on this again 😅

@albi3ro
Copy link
Contributor

albi3ro commented Jun 24, 2024

@astralcai is now looking into this again with #5885 . But not going into the release since we know it's going to break things for catalyst.

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