Skip to content

Conversation

alexanderivrii
Copy link
Member

@alexanderivrii alexanderivrii commented Feb 19, 2025

Summary

Fix two failing tests for the 1.3.3 release:
test.python.quantum_info.operators.channel.test_kraus.TestKraus.test_circuit_init + one other very similar test.

The problem only occurs on aarch64 (arm linux), however the relevant matrix is ill-conditioned on my mac as well: the choi matrix has many eigenvalues that are negative and close to 0 (the "negative" is likely due to numerical imprecision errors), with the numpy.linalg.cond returning np.inf.

By doing a search on the internet, one of the recommended techniques is to apply regularization, in this case adding a small perturbation to the matrix. Update: as per @jakelishman's suggestion, we can in fact perfectly recover the eigenvalues of the original matrix from the matrix with the perturbation.

@alexanderivrii alexanderivrii added the ci: test wheels Run the wheel-build scripts as an additional CI run for this PR label Feb 19, 2025
@alexanderivrii alexanderivrii added this to the 1.3.3 milestone Feb 19, 2025
@alexanderivrii alexanderivrii requested a review from a team as a code owner February 19, 2025 10:22
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@alexanderivrii alexanderivrii removed the ci: test wheels Run the wheel-build scripts as an additional CI run for this PR label Feb 19, 2025
Comment on lines 251 to 252
if apply_perturbation:
values = [v - 1e-10 for v in values]
Copy link
Member

Choose a reason for hiding this comment

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

Possibly we want to do values = values - 1e-10 to use the Numpy vectorised subtraction, and to keep values as an ndarray?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed! Done in 59a7b3c.

@ShellyGarion ShellyGarion added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Feb 19, 2025
@jakelishman jakelishman added the ci: test wheels Run the wheel-build scripts as an additional CI run for this PR label Feb 19, 2025
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.

This seems fine to me now, thanks for digging in to find it.

I've retagged with the wheel test, just to make sure, but assuming that passes on AArch64, let's merge and move on.

@jakelishman
Copy link
Member

@Mergifyio backport main

Copy link
Contributor

mergify bot commented Feb 19, 2025

backport main

✅ Backports have been created

@jakelishman
Copy link
Member

AArch64 passed its tests, so I'll merge.

@jakelishman jakelishman added this pull request to the merge queue Feb 19, 2025
Merged via the queue into Qiskit:stable/1.3 with commit 69eb2aa Feb 19, 2025
27 checks passed
mergify bot pushed a commit that referenced this pull request Feb 19, 2025
* Applying a slight perturbation for ill-conditioned matrices

* Further improving the fix based on discussion with Jake and Gadi, and adding bugfix release note

* Jake's suggestion from code review

(cherry picked from commit 69eb2aa)
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2025
…13889)

* Applying a slight perturbation for ill-conditioned matrices

* Further improving the fix based on discussion with Jake and Gadi, and adding bugfix release note

* Jake's suggestion from code review

(cherry picked from commit 69eb2aa)

Co-authored-by: Alexander Ivrii <alexi@il.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog ci: test wheels Run the wheel-build scripts as an additional CI run for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants