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

Expose cutoff tolerances in Z2Symmetries #7598

Merged
merged 14 commits into from
Feb 17, 2022

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Feb 1, 2022

Summary

Allow setting the cutoff tolerances of the tapered operator in Z2Symmetries. This fixes Qiskit Nature CI, where some workflows pass the tapered operator into the Pauli evolution gate. This fails since #7551 since the tapering creates some spurious complex coefficients from roundoff errors but the Pauli evolution gate does not allow any complex parts at all.

Details and comments

This PR also add a new method the SparsePauliOp.chop to truncate real and imaginary parts of a coefficients individually. That means that coefficients like

[1+1e-17j, 1e-17j]

are truncated to

[1, 0]

This is different to SparsePauliOp.simplify which would simplify the above coefficient to

[1+1e-17j, 0].

This fix could be part of another PR but it's required to properly remove the imaginary parts of the tapered operator so I included it here.

WIP as it misses a test.

@coveralls
Copy link

coveralls commented Feb 1, 2022

Pull Request Test Coverage Report for Build 1860181076

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

Totals Coverage Status
Change from base Build 1859454297: 0.006%
Covered Lines: 52239
Relevant Lines: 62683

💛 - Coveralls

@Cryoris Cryoris changed the title [WIP] Expose cutoff tolerances in Z2Symmetries Expose cutoff tolerances in Z2Symmetries Feb 1, 2022
@Cryoris
Copy link
Contributor Author

Cryoris commented Feb 10, 2022

After discussions with @ikkoham and @jakelishman I updated the PR to do the following now: Leave simplify as is and instead add a new method

def chop(self, tol: float = 1e-14) -> SparsePauliOp:

which chops real and imaginary parts of coefficients that are within an (absolute) tolerance tol of 0. If a coefficient is completely chopped to 0 it is removed from the operator. So, for instance

>>> from qiskit.quantum_info import SparsePauliOp
>>> op = SparsePauliOp(["X", "Y", "Z"], coeffs=[1+1e-17j, 1e-17+1j, 1e-17])
>>> op.simplify()
SparsePauliOp(['X', 'Y'], coeffs=[1.e+00+1.e-17j, 1.e-17+1.e+00j])
>>> op.chop()
SparsePauliOp(['X', 'Y'], coeffs=[1.+0.j, 0.+1.j])

I chose the default 1e-14 a bit at random, since (1) I didn't want to use the SparsePauliOp.atol as thats the tolerance cutoff for the absolute value of a coefficient, not the real and imaginary parts individually and (2) using tol=0.0 wouldn't do anything per default which would be weird too. I'm happy to change the default if you have other preferences!

This method is a bit parallel to simplify now and calling both chop and simplify will copy the coefficients twice but if we want to keep these features (chopping absolute values and chopping real/imag parts) I don't think there's a way around that.

I ran the failing Nature tests locally and with this PR they run again as desired. 🙂

@Cryoris Cryoris added this to the 0.20 milestone Feb 10, 2022
@Cryoris Cryoris added the Changelog: New Feature Include in the "Added" section of the changelog label Feb 10, 2022
ikkoham
ikkoham previously approved these changes Feb 15, 2022
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 think this method is what we want. If we need different tolerance for real and imag, we can add them at that time. I can't think of any use cases at the moment. LGTM

Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

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

Basically lgtm, just some minor observations 👍

qiskit/opflow/primitive_ops/tapered_pauli_sum_op.py Outdated Show resolved Hide resolved
qiskit/opflow/primitive_ops/tapered_pauli_sum_op.py Outdated Show resolved Hide resolved
qiskit/opflow/primitive_ops/tapered_pauli_sum_op.py Outdated Show resolved Hide resolved
qiskit/opflow/primitive_ops/tapered_pauli_sum_op.py Outdated Show resolved Hide resolved
Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch>
@Cryoris Cryoris requested review from ikkoham and mrossinek February 15, 2022 18:33
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I like the new SparsePauliOp.chop - to me, it's definitely a good solution to the general problem, and it's nicely composable for you guys downstream, which hopefully should lead to easier maintenance for everyone. Thanks for sticking with it.

Comment on lines +128 to +138
self._tol = tol

@property
def tol(self):
"""Tolerance threshold for ignoring real and complex parts of a coefficient."""
return self._tol

@tol.setter
def tol(self, value):
"""Set the tolerance threshold for ignoring real and complex parts of a coefficient."""
self._tol = value
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for adding a property to wrap the normal behaviour? It doesn't seem to be doing anything except adding overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it for consistency with tapering_values which is the only other attribute that is settable. Personally I'd also prefer to just have it as public attribute (just self.tol in the init) 🙂

Copy link
Member

Choose a reason for hiding this comment

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

From the user's perspective there wouldn't be any inconsistency unless they're in the habit of requiring del x.tol to raise an error or other weird stuff. But also, I don't really care haha.

@Cryoris Cryoris requested a review from jakelishman February 17, 2022 07:54
Copy link
Member

@jakelishman jakelishman 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! Ikko already approved a slightly earlier version of this, so I'll assume he's fine with the minor performance tweaks, and set this to merge now.

@jakelishman jakelishman added automerge mod: quantum info Related to the Quantum Info module (States & Operators) labels Feb 17, 2022
@mergify mergify bot merged commit 83ac6d7 into Qiskit:main Feb 17, 2022
@Cryoris Cryoris deleted the expose-tolerances-z2symmetries branch February 17, 2022 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants