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 label assigned check in SparsePauliOp #8101

Merged
merged 6 commits into from
Jun 14, 2022
Merged

Conversation

daimurat
Copy link
Member

@daimurat daimurat commented May 23, 2022

Summary

Added a check process discussed in #7916.

Details and comments

  • If the qubit designated from input Pauli list is already assigned (not I), .from_sparse_list raise QiskitError.
  • For performance added do_check flag to be able to turn off the check.

Note: This is a part of QAMP spring 22

@daimurat daimurat requested review from a team and ikkoham as code owners May 23, 2022 13:27
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@@ -636,13 +636,15 @@ def from_sparse_list(obj, num_qubits):
Args:
obj (Iterable[Tuple[str, List[int], complex]]): The list 3-tuples specifying the Paulis.
num_qubits (int): The number of qubits of the operator.
do_checks (bool): The flag of checking if it is already assigned (not I).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I should be checked too, otherwise it's unclear how we interpret using an index multiple times 🙂 Could you add that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @Cryoris.
How about not to being allowed input I instead of checking I ?
I think there seems to be no use case that input I should be applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is true that there is no reason to pass I with sparse format, so I don't think we need to allow I.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's true, strictly speaking there's not reason to pass I. But it might be restrictive to explicitly forbid it because (1) if a user happens to have strings with I in them they would have to clean the string, which can be cumbersome (maybe the string is automatically constructed) and (2) we would have to add a check whether I is included. I think it would be easier for both users and us if we just allow any Pauli string.

We could of course clean the input strings and only store the non-I ones, that might be good though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I feel that it is ok to not check I for the first version. Personally, I am concerned that it would complicate the implementation. As long as we make errors as long as the output is clearly strange (as issue pointed out), we don't have problems.

Copy link
Contributor

@Cryoris Cryoris May 25, 2022

Choose a reason for hiding this comment

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

There are a few ways to implement it which a not complicating the implementation I think 🙂 For example one of the following 2:

  1. Just check if indices contains duplicated (that might be even simpler than the current code 🙂)
if do_checks and np.unique(indices).size == len(indices):
  # raise error
  1. At the beginning, populate labels with None and use the check that you currently have. Then at the end replace all left Nones with "I" 🙂

Copy link
Contributor

@ikkoham ikkoham May 26, 2022

Choose a reason for hiding this comment

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

Both has extra computational cost, but I'd prefer 1 using set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. I fixed the check condition. Could you review it?

@coveralls
Copy link

coveralls commented May 23, 2022

Pull Request Test Coverage Report for Build 2494403645

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 84.425%

Totals Coverage Status
Change from base Build 2493889477: 0.0005%
Covered Lines: 54714
Relevant Lines: 64808

💛 - Coveralls

@ikkoham ikkoham added mod: quantum info Related to the Quantum Info module (States & Operators) QAMP issues and PRs relating to Qiskit Advocates Mentorship Program Changelog: Bugfix Include in the "Fixed" section of the changelog labels May 24, 2022
@ikkoham ikkoham added this to the 0.21 milestone May 28, 2022
ikkoham
ikkoham previously approved these changes May 28, 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.

@daiki0623 Thank you. LGTM. I'll wail @Cryoris's review.

@1ucian0 1ucian0 added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 14, 2022
Cryoris
Cryoris previously approved these changes Jun 14, 2022
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the contribution @daiki0623!

@Cryoris Cryoris added Changelog: New Feature Include in the "Added" section of the changelog automerge and removed Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jun 14, 2022
@Cryoris Cryoris linked an issue Jun 14, 2022 that may be closed by this pull request
@mergify mergify bot merged commit b8ea2b0 into Qiskit:main Jun 14, 2022
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: quantum info Related to the Quantum Info module (States & Operators) QAMP issues and PRs relating to Qiskit Advocates Mentorship Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SparsePauliOp does not yield correct results
6 participants