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

Improve sum op equals #7681

Merged
merged 8 commits into from Mar 10, 2022
Merged

Improve sum op equals #7681

merged 8 commits into from Mar 10, 2022

Conversation

towynlin
Copy link
Contributor

Summary

Fixes the specific problem that was the impetus for #5745.

>>> from qiskit.opflow import I, X
>>> (0 * X + I) == I
True  # was False before this PR
>>> I == (0 * X + I)
True  # was False before this PR

Details and comments

The only reason those equality tests unexpectedly failed before was because the code didn't handle the comparison of a PauliSumOp with a single PauliOp and explicitly returned False when the types differed. Wrapping the PauliOp into a PauliSumOp easily enables the rest of the existing comparison logic.

This is a pretty surgical change compared to the solution proposed about a year-and-a-half ago for qiskit-aqua. There may be more to improve regarding equality testing of SummedOp.

If there's some further clear failure case that requires the originally proposed threshold checking beyond what already happens in reduce, let me know, and I'll be happy to extend this PR — or else we can open another issue for such a case.

@coveralls
Copy link

coveralls commented Feb 18, 2022

Pull Request Test Coverage Report for Build 1962201852

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 83.469%

Totals Coverage Status
Change from base Build 1962064399: 0.001%
Covered Lines: 52437
Relevant Lines: 62822

💛 - Coveralls

@ikkoham
Copy link
Contributor

ikkoham commented Feb 21, 2022

@towynlin Thank you for your contribution.
When I made PauliSumOp, I defined that PauliOp and PauliSumOp are not equal because they have different types. The reason is that introducing this would require similar conversions for other primitive operators (e.g. MatrixOp and CircuitOp), which would be too expensive. However, if you want to do the same conversion for all operators, it might be a good idea to be consistent.

I'd like to hear other people's opinions on this issue.

@jakelishman jakelishman added the mod: opflow Related to the Opflow module label Feb 21, 2022
@woodsp-ibm
Copy link
Member

woodsp-ibm commented Feb 21, 2022

@ikkoham As the PauliSumOp can be thought of a summed list of PauliOps I guess its a question here of when the sum reduces to an effective list of len 1 does it compare to the element that is inside that list. In a similar vein I will note though that [1] == 1 returns False. The other comparisons you refer to do seem to me more types conversions, and I guess this here too in comparing a list to a single element though in practice this is not an expensive conversion like those you refer to. Since both PauliSumOp and PauliOp are operators in our context, and both primitives too, though they have this list relationship perhaps the comparison makes sense to accommodate the sum of single PauliOp? - whereas I think I'd agree in general not wanting to do expensive conversions.

@towynlin
Copy link
Contributor Author

Watching discussion with curiosity. Happy to help in any way I can.

@towynlin
Copy link
Contributor Author

@ikkoham Thank for you sharing the history of how you made PauliSumOp and your rationale at that time. It's a very useful tool, and the performance gains over the prior implementation are indeed impressive. 🙇 I enjoyed reading through the original PR from Nov-Dec 2020 and understand better your intentions.

I think from a developer ergonomics perspective I agree with you @woodsp-ibm that expensive conversions here should be avoided unless more explicitly requested by the user. We wouldn't want to surprise folks with complex computations when they thought they were making a simple comparison.

After thinking about this for the last week of silence on this issue, I believe the best path is to merge this PR as-is if we agree that 0 * X + I == I should evaluate to True, from the perspective of a qiskit user (who reads the Operators docs, basically understands John Watrous's formalism, and, crucially, does not know about the underlying software architecture). I believe that is what such a user would expect and thus makes the most sense here.

Other related issues like comparisons for MatrixOp and CircuitOp could be filed as their own githhub issues and discussed independently.

@ikkoham
Copy link
Contributor

ikkoham commented Mar 1, 2022

To be honest, I don't use this feature (SummedOp of PauliOp) of opflow (I always use PauliSumOp).
So I'm not sure how this equals would be useful for users. But, basically, I'm not strongly opposed to it...

@ikkoham ikkoham added the Changelog: API Change Include in the "Changed" section of the changelog label Mar 1, 2022
@woodsp-ibm
Copy link
Member

This needs a reno release note if this is going to go forwards though.

@woodsp-ibm
Copy link
Member

So I'm not sure how this equals would be useful for users. But, basically, I'm not strongly opposed to it...

@ikkoham I think this just makes sure some of the basics work as expected. I'll go ahead and approve to try and get this finished off - it seems that this is ok for you, in that you are not strongly opposed, but this is to do a final check with you .

Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

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

Thank you. I thought this was a good user request. LGTM.

@mergify mergify bot merged commit 9f867e9 into Qiskit:main Mar 10, 2022
@towynlin towynlin deleted the improve-sum-op-equals branch March 11, 2022 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog mod: opflow Related to the Opflow module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants