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

Remove the classical_function module and all tweedledum-related code #13815

Merged
merged 7 commits into from
Feb 27, 2025

Conversation

gadial
Copy link
Contributor

@gadial gadial commented Feb 10, 2025

Summary

Removes from qiskit the classical_function module and all code that relies on the tweedledum library.
Closes #13755

Details and comments

The tweedledum library is not compatible with new versions of Python, so reliance on it has to be dropped in Qiskit 2.0. Since the classical_function module basically servers as a qiskit wrapper for tweedledum functionality, we remove it from qiskit, after its deprecation in #13786.

To preserve the main functionality of classical_function, i.e. the ability to generate phase and bit-flip oracles from boolean expressions, we add a simple, tweedledum-independent implementation in #13769, which is placed in qiskit.synthesis.boolean instead of the classical_function module. This PR relies on #13769 being merged, otherwise the existing tests for PhaseOracle will not pass.

@gadial gadial added the Changelog: Removal Include in the Removed section of the changelog label Feb 10, 2025
@gadial gadial added this to the 2.0.0 milestone Feb 10, 2025
@gadial gadial self-assigned this Feb 10, 2025
@gadial gadial requested review from nonhermitian and a team as code owners February 10, 2025 07:21
@qiskit-bot
Copy link
Collaborator

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

Copy link
Member

@ShellyGarion ShellyGarion 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. I only wonder if we should keep some of the removed tests?

@@ -1371,105 +1370,5 @@ def test_cswap_reg_reg_inv(self):
self.assertEqual(instruction_set[1].qubits, (self.qr[1], self.qr2[1], self.qr3[1]))
self.assertEqual(instruction_set[2].operation.params, [])


class TestStandardMethods(QiskitTestCase):
Copy link
Member

Choose a reason for hiding this comment

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

can we do these tests with PhaseOracle w/o tweedledum ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, actually tweedledum was not really needed there. I returned the test.

return a and b and not c

circuit = grover_oracle.synth(registerless=False)
self.assertEqual(str(circuit_drawer(circuit, output="text", initial_state=True)), expected)
Copy link
Member

Choose a reason for hiding this comment

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

can we still do this tests with grover or phase oracle w/o tweedledum ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make sense, since this is a test for registerless synthesis, which is currently not supported by the oracles.

@1ucian0 1ucian0 self-assigned this Feb 20, 2025
@1ucian0
Copy link
Member

1ucian0 commented Feb 24, 2025

on hold until #13769 merges

@1ucian0 1ucian0 added the on hold Can not fix yet label Feb 24, 2025
@ShellyGarion ShellyGarion removed the on hold Can not fix yet label Feb 25, 2025
@mtreinish mtreinish changed the title [WIP] Remove the classical_function module and all tweedledum-related code Remove the classical_function module and all tweedledum-related code Feb 26, 2025
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, just one note on the reno 🙂 (also black is failing)

gadial and others added 2 commits February 26, 2025 18:31
@coveralls
Copy link

coveralls commented Feb 26, 2025

Pull Request Test Coverage Report for Build 13551023854

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.3%) to 88.129%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.23%
crates/qasm2/src/lex.rs 4 92.48%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 13544415862: 0.3%
Covered Lines: 77532
Relevant Lines: 87976

💛 - Coveralls

Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

LGTM. I'll let @Cryoris or @1ucian0 merge it. Thanks @gadial !

@1ucian0 1ucian0 added this pull request to the merge queue Feb 27, 2025
Merged via the queue into Qiskit:main with commit b83298b Feb 27, 2025
19 checks passed
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Mar 1, 2025
In Qiskit#13815 the usage of the tweedledum library was removed from Qiskit.
The classes that used this functionality was deprecated in 1.4.0, but the
optional library checker was not deprecated. These optionals checkers
are part of the public API and would have needed a deprecation before
removal as there may be downstream users of the checker. This commit
restores the checker, but alters the docstring to make it clear that
tweedledum isn't used by Qiskit anymore. If we want to remove the
checker in 3.0 we can deprecate it in 2.1.0 or another release in the
2.x series. Although, there is basically no real cost for keeping it
around so it's probably not worth the effort.
github-merge-queue bot pushed a commit that referenced this pull request Mar 1, 2025
In #13815 the usage of the tweedledum library was removed from Qiskit.
The classes that used this functionality was deprecated in 1.4.0, but the
optional library checker was not deprecated. These optionals checkers
are part of the public API and would have needed a deprecation before
removal as there may be downstream users of the checker. This commit
restores the checker, but alters the docstring to make it clear that
tweedledum isn't used by Qiskit anymore. If we want to remove the
checker in 3.0 we can deprecate it in 2.1.0 or another release in the
2.x series. Although, there is basically no real cost for keeping it
around so it's probably not worth the effort.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Removal Include in the Removed section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove tweedledum from qiskit-sdk
6 participants