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

Legacy operator arithmetic deprecation #6287

Merged
merged 33 commits into from
Oct 3, 2024
Merged

Conversation

mudit2812
Copy link
Contributor

@mudit2812 mudit2812 commented Sep 19, 2024

Context:
Legacy op math is being deprecated.

Description of the Change:

  • qml.ops.Hamiltonian is deprecated.

  • qml.operation.Tensor is deprecated.

  • qml.operation.disable_new_opmath and qml.operation.enable_new_opmath (as well as the context managers) are deprecated.

  • qml.operation.convert_to_legacy_H is deprecated.

  • PauliWord.hamiltonian and PauliSentence.hamiltonian is deprecated.

  • qml.pauli.simplify is deprecated.

  • Removed use_legacy_opmath and use_new_opmath fixtures. Instead, we use either legacy_opmath_only, new_opmath_only, or no fixture, as appropriate for the tests.

  • Added warning suppression for all the above deprecation warnings to pytest.ini. Additionally, I also added the same warning suppressions to conftest.py for the case when --disable-opmath=True, since the warning filters in pytest.ini only suppress warnings that come from inside tests, not ones that come from outside the tests, such as when deprecated code is used for creating parameters for a test.

Benefits:

Possible Drawbacks:
In some instances, there will be a LOT of deprecation warnings at the same time. The most impactful example of this if if using qml.qchem.molecular_hamiltonian, qml.qchem.qubit_observable. These using simplify and PauliSentence.hamiltonian with legacy op math, so there will be up to 4 warnings at once when using these.

Additionally, qml.qchem.tapering.symmetry_generators and qml.qchem.tapering.clifford use PauliSentence.hamiltonian when legacy op math is enabled, so there will be up to 3 warnings at once when using these.

Related GitHub Issues:

I will run the test suite with disable_new_opmath set to True once before merging to confirm that legacy op math tests do not fail.

[sc-71940] [sc-66727] [sc-66724]

@mudit2812 mudit2812 marked this pull request as ready for review September 19, 2024 21:11
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.39%. Comparing base (d9e833a) to head (455c9a9).
Report is 339 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6287      +/-   ##
==========================================
- Coverage   99.70%   99.39%   -0.32%     
==========================================
  Files         445      445              
  Lines       42260    42271      +11     
==========================================
- Hits        42137    42016     -121     
- Misses        123      255     +132     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lillian542 lillian542 self-requested a review September 20, 2024 19:50
@PietropaoloFrisoni
Copy link
Contributor

Should we also replace use_legacy_opmath with legacy_opmath_only in this test?

Copy link
Contributor

@astralcai astralcai left a comment

Choose a reason for hiding this comment

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

There are a lot more than can be simplified around our testing with legacy opmath. Although I'm ok to approve if we're under pressure of time to merge this by today, since we're removing legacy opmath completely in a few months anyway?

doc/development/deprecations.rst Show resolved Hide resolved
tests/fermi/test_bravyi_kitaev.py Show resolved Hide resolved
tests/devices/test_preprocess.py Outdated Show resolved Hide resolved
tests/fermi/test_bravyi_kitaev.py Show resolved Hide resolved
tests/ops/op_math/test_linear_combination.py Outdated Show resolved Hide resolved
Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

I didn't get through that much of it but here are a few comment for now :)

doc/development/deprecations.rst Outdated Show resolved Hide resolved
doc/news/new_opmath.rst Outdated Show resolved Hide resolved
pennylane/operation.py Show resolved Hide resolved
@mudit2812
Copy link
Contributor Author

Looks like running some tests only when legacy op math is active has caused some humps with codecov. Not sure if it should be addressed or not.

Copy link
Contributor

@astralcai astralcai left a comment

Choose a reason for hiding this comment

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

This looks like a painfully large amount of work, thank you for doing this 🚀

@PietropaoloFrisoni
Copy link
Contributor

PietropaoloFrisoni commented Oct 3, 2024

To fix the error in:

tests/optimize/test_momentum_qng.py::TestOptimize::test_single_qubit_vqe_using_expval_h_multiple_input_params (added today) and tests/optimize/test_qng.py::TestOptimize::test_single_qubit_vqe_using_expval_h_multiple_input_params

I guess we can safely remove the recwarn parameter. I am not sure about the other failures

mudit2812 and others added 3 commits October 3, 2024 16:24
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni left a comment

Choose a reason for hiding this comment

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

I can only guess how daunting this deprecation was, thanks a lot! I see that there is still a test_use_legacy_opmath_fixture entry in the test duration files, but I don't think this is an issue

Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

One non-blocking comment, this looks good to me! 🚀

doc/news/new_opmath.rst Outdated Show resolved Hide resolved
@mudit2812
Copy link
Contributor Author

It looks like I just need to add back some of the duplicate tests we had for legacy opmath tests to pass. For some reason, the legacy opmath enable only happens after all the parameters for parametrized tests are created, so we're ending up in a situation where we create LinearCombinations and Prods when Hamiltonians and Tensors are expected.

@mudit2812
Copy link
Contributor Author

mudit2812 commented Oct 3, 2024

Confirmation that all tests pass with --disable-opmath=True here.

@Alex-Preciado
Copy link
Contributor

Proceeding to force merge after discussing with @mudit2812 !! Context: Tests that used to run in CI for legacy opmath have been changed to only run in the Plugin Test Matrix

@Alex-Preciado Alex-Preciado merged commit 9f4e67e into master Oct 3, 2024
36 of 37 checks passed
@Alex-Preciado Alex-Preciado deleted the deprecate-legacy-opmath branch October 3, 2024 22:15
@lillian542 lillian542 mentioned this pull request Nov 12, 2024
10 tasks
lillian542 added a commit that referenced this pull request Nov 17, 2024
**Context:**
It's time to fully remove the legacy operator arithmetic (`Tensor` and
`qml.ops.Hamiltonian`, and all of the structure surrounding them).

**Description of the Change:**

- [x] remove `qml.ops.Hamiltonian`
- [x]  remove `Tensor`
- [x]  remove `disable_` and `enable_new_opmath`
- [x]  remove `operation.convert_to_legacy_H`
- [x]  remove `qml.pauli.simplify`
- [x] remove `PauliWord.hamiltonian` and `PauliSentence.hamiltonian`,
and any other functionality in the pauli module that supported legacy
opmath
- [x] remove any if/else handling in place to deal with legacy opmath
through the code base
- [x] remove `legacy_opmath_only`, `new_opmath_only` and
`use_legacy_and_new_opmath` test fixtures, and all irrelevant tests
- [x] remove the warning suppressions added in
[PR6287](#6287)
- [x]  remove the legacy test fixture (--disable-opmath)


[sc-77523]

---------

Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Astral Cai <astral.cai@xanadu.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants