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

Deprecate qiskit.circuit.classicalfunction #13786

Merged
merged 16 commits into from
Feb 19, 2025

Conversation

gadial
Copy link
Contributor

@gadial gadial commented Feb 5, 2025

Summary

This PR deprecates the ClassicalFunction class and related classical_function method, to be removed in 2.0.

Partially addresses #13755

Details and comments

ClassicalFunction is a useful class, but it relies on the tweedledum library which is dropped from qiskit in 2.0. Most of the functionality enabled by ClassicalFunction is also supplied by BooleanExpression that is made independent from tweedledum in #13769 and it's not clear if there is demand for the extra functionality at this stage.

@gadial gadial requested a review from a team as a code owner February 5, 2025 10:10
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@gadial gadial added this to the 1.4.0 milestone Feb 5, 2025
@coveralls
Copy link

coveralls commented Feb 5, 2025

Pull Request Test Coverage Report for Build 13235330308

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 8 (0.0%) changed or added relevant lines in 3 files are covered.
  • 1405 unchanged lines in 44 files lost coverage.
  • Overall coverage decreased (-0.4%) to 88.267%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/classicalfunction/init.py 0 2 0.0%
qiskit/circuit/classicalfunction/boolean_expression.py 0 3 0.0%
qiskit/circuit/classicalfunction/classicalfunction.py 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/gate_direction.rs 1 97.34%
qiskit/pulse/schedule.py 1 88.42%
qiskit/utils/parallel.py 1 80.7%
qiskit/pulse/instruction_schedule_map.py 1 95.9%
qiskit/providers/fake_provider/fake_openpulse_3q.py 1 92.86%
qiskit/transpiler/passes/scheduling/alignments/check_durations.py 1 77.78%
qiskit/circuit/classicalfunction/classicalfunction.py 1 0.0%
crates/accelerate/src/euler_one_qubit_decomposer.rs 1 91.5%
crates/accelerate/src/twirling.rs 1 97.62%
qiskit/circuit/classicalfunction/init.py 1 0.0%
Totals Coverage Status
Change from base Build 13154732632: -0.4%
Covered Lines: 78616
Relevant Lines: 89066

💛 - Coveralls

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.

Thanks for taking this on, I'm happy to see this cleaned up for 2.0! 🙂

It seems there is still one call to classical function in the visualization tests, which is not caught and causes the CI to fail.

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.

Some small comments, otherwise LGTM 👍🏻

@Cryoris Cryoris changed the title Deprecate ClassicalFunction Deprecate qiskit.circuit.classicalfunction Feb 5, 2025
@gadial gadial requested a review from nonhermitian as a code owner February 6, 2025 07:53
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.

Just making sure: in this PR we raise deprecation warnings, and the new API will be introduced in qiskit 2.0 ?
Should there be another PR for removing ClassicalFunction for qiskit 2.0, since it's not part of #13769?

@ShellyGarion
Copy link
Member

ShellyGarion commented Feb 9, 2025

I also found tweedledum here:

.. py:data:: HAS_TWEEDLEDUM

should it be removed / deprecated?

See also this test file:
https://github.com/Qiskit/qiskit/blob/main/test/python/circuit/test_extensions_standard.py

@gadial
Copy link
Contributor Author

gadial commented Feb 10, 2025

Just making sure: in this PR we raise deprecation warnings, and the new API will be introduced in qiskit 2.0 ? Should there be another PR for removing ClassicalFunction for qiskit 2.0, since it's not part of #13769?

Yes, we'll add the new API in 2.0.

I think we can use #13769 to remove ClassicalFunction, even though I usually try to keep PR's centered around one change.

@gadial
Copy link
Contributor Author

gadial commented Feb 10, 2025

I also found tweedledum here:

.. py:data:: HAS_TWEEDLEDUM

should it be removed / deprecated?
See also this test file: https://github.com/Qiskit/qiskit/blob/main/test/python/circuit/test_extensions_standard.py

It should definitely be removed, but I don't think we need deprecation warning here.

@1ucian0
Copy link
Member

1ucian0 commented Feb 10, 2025

This one should be against stable/1.4

@gadial gadial changed the base branch from main to stable/1.4 February 11, 2025 09:58
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.

This deprecation is a bit finnicky. Our deprecation rules are that

  1. an alternative implementation must be available for 1 minor release, where no DeprecationWarning is issued (PendingDeprecationWarning is fine)
  2. then, the DeprecationWarning must be issued for 1 minor release
  3. finally, we can remove the feature

Here we cannot point users to the BitFlipOracle, which doesn't exist yet, and we cannot issue a deprecation warning inside PhaseOracle, which doesn't have a deprecation-free alternative. In a perfect world we would have introduced a tweedledum-free version of PhaseOracle and the BitFlipOracle in 1.3 already, deprecate the classicalfunction in 1.4 and remove in 2.0.

To move forward while still being true to the deprecation policy, I'd suggest we'd

  • just point to PhaseOracle as alternative path for everything (this has existed for long enough)
  • don't issue any warnings when using PhaseOracle
  • tell users how they could get a boolean expression/bitflip oracle from a PhaseOracle

@deprecate_func(
since="1.4",
removal_timeline="in Qiskit 2.0",
additional_msg="Use `PhaseOracle` or `BitFlipOracle` instead",
Copy link
Contributor

Choose a reason for hiding this comment

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

The BitFlipOracle doesn't exist in Qiskit 1.4, we need to remove mentioning it from the deprecation messages. Since the PhaseOracle is not a direct replacement of the classical functions but requires extra steps, it might be good to tell the users how to get there

Suggested change
additional_msg="Use `PhaseOracle` or `BitFlipOracle` instead",
additional_msg=(
"Use the PhaseOracle instead, which can be turned into a "
"bit-flip oracle by applying Hadamard gates on the target "
"qubit before and after the instruction."
),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4510aee

Comment on lines +11 to +12
is going to have a `tweedledum` independent implementation, and
the :class:`.BitFlipOracle` which will be added in Qiskit 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add a code snippet as example if we want to be extra nice to the users 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that, and maybe the release notes would be a perfect place to include an example on how to convert the phase oracle to the bit-flip oracle.

DeprecationWarning,
expected_regex="BooleanExpression`` is deprecated as of qiskit 1.4",
):
oracle = PhaseOracle(expression)
Copy link
Contributor

Choose a reason for hiding this comment

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

The PhaseOracle is not getting deprecated, it should not emit a deprecation warning. Instead we can filter the warning in the PhaseOracle code in 1.4 until we get the tweedledum-free version in 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4510aee

ShellyGarion
ShellyGarion previously approved these changes Feb 14, 2025
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. do you have any last comments @Cryoris ?

Comment on lines 67 to 69
import warnings

warnings.simplefilter("ignore", DeprecationWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will enable a global filter, we should use

with warnings.catch_warnings(action="ignore", category=DeprecationWarning):
   # code here...

🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b4878c6


.. code-block:: python

phase_flip_oracle = PhaseOracle(bool_expr)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's nice! What do you think of making this a fully executable snippet with some example boolean expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b4878c6

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.

Thanks for all the work!

@Cryoris Cryoris added this pull request to the merge queue Feb 18, 2025
github-merge-queue bot pushed a commit that referenced this pull request Feb 18, 2025
* Deprecate `ClassicalFunction`

* Changes according to PR review

* Update releasenotes/notes/deprecate-classical-function-3bf44ef26d984366.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update qiskit/circuit/classicalfunction/__init__.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Update releasenotes/notes/deprecate-classical-function-3bf44ef26d984366.yaml

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* Test fix

* Deprecate `BooleanExpression` as well

* Linting

* Test fix due to deprecation

* Update the release note

* Test message fix

* Update deprecation message and ignore deprecation warning in the non-deprecated PhaseOracle

* Added an explanation on how to create a bit flip oracle from a phase flip oracle

* Fixes according to PR review

* Typo

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2025
@ShellyGarion ShellyGarion added this pull request to the merge queue Feb 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2025
@Cryoris Cryoris added this pull request to the merge queue Feb 18, 2025
@Cryoris
Copy link
Contributor

Cryoris commented Feb 18, 2025

Third try to really be sure it's not a flake..

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2025
@ElePT ElePT added this pull request to the merge queue Feb 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 19, 2025
@ElePT
Copy link
Contributor

ElePT commented Feb 19, 2025

I just inspected the backwards compatibility tests and found the source of the problems we are seeing:
qiskit.qpy.exceptions.QpyError: 'Incompatible symengine version 0.14 used to generate the QPY payload'. We need to see why symengine 0.14 is getting installed where it shouldn't be. [Edit: earlier qiskit versions don't have the pin on the symengine version, that's why].

@ElePT ElePT added this pull request to the merge queue Feb 19, 2025
Merged via the queue into Qiskit:stable/1.4 with commit 71eb989 Feb 19, 2025
16 checks passed
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
None yet
Development

Successfully merging this pull request may close these issues.

8 participants