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 qiskit.extensions #11488

Merged
merged 13 commits into from
Jan 17, 2024
Merged

Remove qiskit.extensions #11488

merged 13 commits into from
Jan 17, 2024

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Jan 4, 2024

Summary

Remove the (sort of) deprecated qiskit.extensions module for Qiskit 1.0 and remove deprecated circuit methods (such as fredkin). I'm not sure if the reno should be more detailed, here I'm referring to the 0.45 reno, but we could also repeat that information here.

A separate PR will deprecate qiskit.extensions on the 0.46 branch.

Details and comments

  • The prepare_state method should've been moved to quantumcircuit.py in the deprecation PR but I overlooked it
  • Also updates some comments to not use the terminology "extensions" anymore
  • Misses updating the reference images for the matplotlib tests, but I can't generate them locally so I'm misusing the CI for this

@Cryoris Cryoris added the Changelog: Removal Include in the Removed section of the changelog label Jan 4, 2024
@Cryoris Cryoris added this to the 1.0.0 milestone Jan 4, 2024
@Cryoris Cryoris requested review from nonhermitian and a team as code owners January 4, 2024 15:06
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

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 largely looks sensible to me, thanks. The lint failures are all trivial unused imports, and I commented on the two docs problems.

releasenotes/notes/remove-extensions-ce520ba419c93c58.yaml Outdated Show resolved Hide resolved
Comment on lines -1467 to +1469
# DiagonalGate (and a bunch of the qiskit.extensions gates) have non-deterministic
# DiagonalGate (and some of the qiskit.circuit.library gates) have non-deterministic
Copy link
Member

Choose a reason for hiding this comment

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

lol, I'm betting I wrote that "a bunch"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha I proposed to tone it down since now it's only a few gates from the standard library (whereas before it was lot of the ones in extension) 😛

Copy link
Member

Choose a reason for hiding this comment

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

Haha yeah, that's fine. I'm mostly laughing at my terminology - I think I'm the only one of us who talks like that, and almost certainly the only one who writes code like that.

docs/apidoc/extensions.rst Outdated Show resolved Hide resolved
qiskit/circuit/quantumcircuit.py Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jan 4, 2024

Pull Request Test Coverage Report for Build 7556014030

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 89.404%

Totals Coverage Status
Change from base Build 7552937080: 0.08%
Covered Lines: 59224
Relevant Lines: 66243

💛 - Coveralls

jakelishman
jakelishman previously approved these changes Jan 5, 2024
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.

Thanks for this - the release note is much more helpful now!

I guess the MPL test of "barrier-like objects" is no longer doing as much of its job (now it only tests real barriers), but I'm not beaten up about that - I'm not convinced that a barrier is the best drawing choice for an arbitrary "directive" anyway.

Comment on lines -4457 to +4454
params: Sequence[complex] | str | int,
params: Statevector | Sequence[complex] | str | int,
Copy link
Member

Choose a reason for hiding this comment

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

Sequence[complex] is overspecified (my pet complaint that Sequence is pretty much never correct as a type hint), but leaving aside .index and .count, Statevector would actually have already been covered by a type-hint of numpy.typing.ArrayLike, which is more appropriate.

That's not important, though, I just like complaining about Sequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this would be nice to rework consistently in all type hints (though numpy.typing.ArrayLike is a bit long 😛)

Comment on lines -1467 to +1469
# DiagonalGate (and a bunch of the qiskit.extensions gates) have non-deterministic
# DiagonalGate (and some of the qiskit.circuit.library gates) have non-deterministic
Copy link
Member

Choose a reason for hiding this comment

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

Haha yeah, that's fine. I'm mostly laughing at my terminology - I think I'm the only one of us who talks like that, and almost certainly the only one who writes code like that.

@jakelishman jakelishman added this pull request to the merge queue Jan 5, 2024
@jakelishman jakelishman removed this pull request from the merge queue due to a manual request Jan 5, 2024
@jakelishman
Copy link
Member

Actually, I'm removing this from the merge queue and placing "on hold" until the deprecations at least have a PR open, because it's not eligible for merge without them. It's ready to merge as soon as that's in place, though.

@jakelishman jakelishman added the on hold Can not fix yet label Jan 5, 2024
@Cryoris
Copy link
Contributor Author

Cryoris commented Jan 5, 2024

Deprecation PR is coming up 🙂

@Cryoris
Copy link
Contributor Author

Cryoris commented Jan 8, 2024

Also on hold due to Qiskit/qiskit-aer#2023.

@jakelishman
Copy link
Member

Yeah, it's not great that our CI is configured in such a way that we didn't notice that Aer fails to import with this change in place.

@Cryoris Cryoris requested a review from a team as a code owner January 17, 2024 09:05
@Cryoris Cryoris removed the on hold Can not fix yet label Jan 17, 2024
@Cryoris
Copy link
Contributor Author

Cryoris commented Jan 17, 2024

Removing on hold as Aer 0.13.2 has been released!

jakelishman
jakelishman previously approved these changes Jan 17, 2024
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.

Thanks for getting this sorted!

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.

Thanks for clearing up the test skips. I scanned through again, and it looks like it's all back in the state that I originally approved.

@jakelishman jakelishman added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
@Cryoris Cryoris added this pull request to the merge queue Jan 17, 2024
Merged via the queue into Qiskit:main with commit 27761bd Jan 17, 2024
13 checks passed
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.

4 participants