Skip to content

Conversation

@Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Nov 20, 2025

Summary

Deprecate the legacy .npy format for serializing the basic approximations in Solovay Kitaev. This internally relies on pickle and could be potentially unsafe.

@Cryoris Cryoris added this to the 2.3.0 milestone Nov 20, 2025
@Cryoris Cryoris requested a review from a team as a code owner November 20, 2025 20:40
@Cryoris Cryoris added the Changelog: Deprecation Include in "Deprecated" section of changelog label Nov 20, 2025
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

Comment on lines 259 to 263
@deprecate_func(
since="2.1",
additional_msg="Use query_basic_approximation instead, which takes a Gate or matrix "
"as input and returns a QuantumCircuit object.",
pending=True,
)
Copy link
Member

Choose a reason for hiding this comment

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

Need to change to "since 2.3" now, I think

Comment on lines 77 to 78
with self.assertWarns(DeprecationWarning):
self.basic_approx = generate_basic_approximations([HGate(), TGate(), TdgGate()], 3)
Copy link
Member

Choose a reason for hiding this comment

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

Can you include the warning message in an assertWarnsRegex? Even from this PR, I can't really see why generate_basic_approximations should now raise a DeprecationWarning (isn't the only thing we newly raise related to loading them?).

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'll add the messages 👍🏻 I added the deprecation to both loading and generation (since nothing will be able to consume the deprecated format in the future)

Comment on lines 157 to 159
with self.assertWarns(DeprecationWarning):
basic_approx = generate_basic_approximations(["h", "t", "s"], 3)
sk = SolovayKitaev(3, basic_approx)
Copy link
Member

Choose a reason for hiding this comment

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

Which of these two calls is suppose to raise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You called out my laziness -- both are 🙂 Fixed now!

Comment on lines 247 to 248
with self.assertWarns(DeprecationWarning):
reference = SolovayKitaev(basic_approximations=gate_approx_library)(circuit)
Copy link
Member

Choose a reason for hiding this comment

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

Which of the calls is supposed to raise? (same question every time we have this pattern.) Can we make the warning catch stricter?

@coveralls
Copy link

coveralls commented Nov 20, 2025

Pull Request Test Coverage Report for Build 19575376142

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 18 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.01%) to 88.214%

Files with Coverage Reduction New Missed Lines %
crates/circuit/src/parameter/parameter_expression.rs 1 82.3%
crates/circuit/src/parameter/symbol_expr.rs 2 73.14%
crates/qasm2/src/lex.rs 3 92.03%
crates/qasm2/src/parse.rs 12 97.09%
Totals Coverage Status
Change from base Build 19550401211: -0.01%
Covered Lines: 94300
Relevant Lines: 106899

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Deprecation Include in "Deprecated" section of changelog

Projects

Status: Ready

Development

Successfully merging this pull request may close these issues.

4 participants