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

Removed code deprecated in qiskit-terra 0.21, released on June 2022 #10754

Merged
merged 22 commits into from
Oct 18, 2023

Conversation

Raghav-Bell
Copy link
Contributor

@Raghav-Bell Raghav-Bell commented Sep 1, 2023

It fixes #10748

Summary

Details and comments

Please tag it as Changelog: Removal

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Sep 1, 2023
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @ElePT
  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989
  • @woodsp-ibm

@Raghav-Bell
Copy link
Contributor Author

tox test fails on test\benchmarks\scheduling_passes.py as it only contains check for TimeUnitConversion . Should we keep test\benchmarks\scheduling_passes.py or not ? also i have removed most of the tests for depreciated or removed classes. Please let me know if i am missing something.

@1ucian0 1ucian0 self-assigned this Sep 1, 2023
@Cryoris Cryoris changed the title Removed code and tests depreciated in 0.21 Removed code and tests deprecated in 0.21 Sep 1, 2023
@ElePT ElePT added the Changelog: Removal Include in the Removed section of the changelog label Sep 1, 2023
@woodsp-ibm
Copy link
Member

I am unsure about optimizers/spsa - mainly in regards of whatever upgrade note you do for the release note (which has yet to be included here). The optimizers are part of algorithms - I did have a prior issue #9866 that included addressing spsa but when the whole of algorithms was deprecated in the last release I simply closed it. I know in other similar issues that @1ucian0 created any deprecation removal for algorithms was dropped - maybe that's the simplest course here @1ucian0? - not to change spsa here given the whole module its part of is already deprecated - that will avoid any issues around including it or not in the release note

@ElePT
Copy link
Contributor

ElePT commented Sep 1, 2023

I'm with @woodsp-ibm, I think it's best to leave algorithms-related code untouched until its removal.

@1ucian0
Copy link
Member

1ucian0 commented Sep 1, 2023

I agree that leaving out qiskit.algorithms from this removal is probably the best option (the original list was created by a program).

My apologies for including it originally.

@1ucian0 1ucian0 added this to the 0.45.0 milestone Sep 1, 2023
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

I moving this PR as draft, as it seems to need some extra work before reviewing. Don't forget to add a release note:

create a release note in the category upgrade:. If you can include an example with an alternative for user to migrate to the new code, as this change might break users code.

@1ucian0 1ucian0 marked this pull request as draft September 4, 2023 08:52
@1ucian0
Copy link
Member

1ucian0 commented Sep 14, 2023

Take a look to the conflicts! When ready for review, hit Ready for review.

@Raghav-Bell
Copy link
Contributor Author

@1ucian0 These conflicts are due to recent merge of PR #10753 as in the PR #10753 classes ALAPSchedule, AlignMeasures, DynamicalDecoupling are changed which are deprecated in 0.21 (removed with merge of this PR). I think we can merge this PR. Please let me know what do you think on this ?

@Raghav-Bell Raghav-Bell marked this pull request as ready for review September 14, 2023 16:43
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@1ucian0
Copy link
Member

1ucian0 commented Sep 16, 2023

These conflicts are due to recent merge of PR #10753 as in the [...] Please let me know what do you think on this ?

I mean, by "take a look" I wanted to say "resolve the conflicts". Sometimes my word choice is not the best. My bad.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6244303397

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 99 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.1%) to 87.167%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
qiskit/transpiler/passes/scheduling/alignments/check_durations.py 2 72.0%
crates/qasm2/src/lex.rs 3 92.17%
qiskit/pulse/library/waveform.py 3 93.75%
qiskit/transpiler/passes/scheduling/base_scheduler.py 33 0.0%
qiskit/transpiler/passes/scheduling/alignments/reschedule.py 57 24.05%
Totals Coverage Status
Change from base Build 6240841904: -0.1%
Covered Lines: 73897
Relevant Lines: 84776

💛 - Coveralls

@1ucian0 1ucian0 changed the title Removed code and tests deprecated in 0.21 Removed code deprecated in qiskit-terra 0.21, released on June 2022 Sep 22, 2023
Copy link
Member

@1ucian0 1ucian0 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 care of this! It is a complicated one. I think some of the tests should be migrated instead of removed. Would you like to have a shoot at that?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not fully sure, but I think these tests should be migrated, as they are test/benchmarks

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 think they are already migrated here in benchmarks/scheduling_passes.py.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, these tests should be migrated to the new workflow, I think.

Copy link
Contributor Author

@Raghav-Bell Raghav-Bell Sep 23, 2023

Choose a reason for hiding this comment

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

I have checked similiar tests in benchmarks/transpiler_levels.py. Can you please elaborate about this? Should we add a github workflow for them ?

qiskit/execute_function.py Outdated Show resolved Hide resolved
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

I migrated the benchmark tests to the new style. The rest, let's merge. Thanks!

@1ucian0 1ucian0 enabled auto-merge October 18, 2023 11:15
@1ucian0 1ucian0 added this pull request to the merge queue Oct 18, 2023
Merged via the queue into Qiskit:main with commit d86e708 Oct 18, 2023
13 checks passed
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Nov 2, 2023
In Qiskit#10754 3 legacy scheduling passes were accidently deleted. These
passes were incorrectly identified as deprecated, however they were
never marked as deprecating just pending future deprecation. They were
intended to be be promoted from a pending deprecation to a full
deprecation in Qiskit#8023 but we never took that step because there were
objections at the time as they still served a purpose. Qiskit#10754 likely
missed this as the only indication in the deprecation decorator was a
kwarg that said `pending=True`, and this was the only indication that
these passes weren't actually deprecated yet. This commit restores these
passes on the 0.45.0 branch in the interest of unblocking the 0.45.0
release ASAP. We can handle forward porting this PR to main as needed
after the 0.45.0 release is tagged.
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Nov 2, 2023
In Qiskit#10754 3 legacy scheduling passes were accidently deleted. These
passes were incorrectly identified as deprecated, however they were
never marked as deprecating just pending future deprecation. They were
intended to be be promoted from a pending deprecation to a full
deprecation in Qiskit#8023 but we never took that step because there were
objections at the time as they still served a purpose. Qiskit#10754 likely
missed this as the only indication in the deprecation decorator was a
kwarg that said `pending=True`, and this was the only indication that
these passes weren't actually deprecated yet. This commit restores these
passes on the 0.45.0 branch in the interest of unblocking the 0.45.0
release ASAP. We can handle forward porting this PR to main as needed
after the 0.45.0 release is tagged.
github-merge-queue bot pushed a commit that referenced this pull request Nov 3, 2023
In #10754 3 legacy scheduling passes were accidently deleted. These
passes were incorrectly identified as deprecated, however they were
never marked as deprecating just pending future deprecation. They were
intended to be be promoted from a pending deprecation to a full
deprecation in #8023 but we never took that step because there were
objections at the time as they still served a purpose. #10754 likely
missed this as the only indication in the deprecation decorator was a
kwarg that said `pending=True`, and this was the only indication that
these passes weren't actually deprecated yet. This commit restores these
passes on the 0.45.0 branch in the interest of unblocking the 0.45.0
release ASAP. We can handle forward porting this PR to main as needed
after the 0.45.0 release is tagged.
mergify bot pushed a commit that referenced this pull request Nov 3, 2023
In #10754 3 legacy scheduling passes were accidently deleted. These
passes were incorrectly identified as deprecated, however they were
never marked as deprecating just pending future deprecation. They were
intended to be be promoted from a pending deprecation to a full
deprecation in #8023 but we never took that step because there were
objections at the time as they still served a purpose. #10754 likely
missed this as the only indication in the deprecation decorator was a
kwarg that said `pending=True`, and this was the only indication that
these passes weren't actually deprecated yet. This commit restores these
passes on the 0.45.0 branch in the interest of unblocking the 0.45.0
release ASAP. We can handle forward porting this PR to main as needed
after the 0.45.0 release is tagged.

(cherry picked from commit aa272e9)
github-merge-queue bot pushed a commit that referenced this pull request Nov 3, 2023
In #10754 3 legacy scheduling passes were accidently deleted. These
passes were incorrectly identified as deprecated, however they were
never marked as deprecating just pending future deprecation. They were
intended to be be promoted from a pending deprecation to a full
deprecation in #8023 but we never took that step because there were
objections at the time as they still served a purpose. #10754 likely
missed this as the only indication in the deprecation decorator was a
kwarg that said `pending=True`, and this was the only indication that
these passes weren't actually deprecated yet. This commit restores these
passes on the 0.45.0 branch in the interest of unblocking the 0.45.0
release ASAP. We can handle forward porting this PR to main as needed
after the 0.45.0 release is tagged.

(cherry picked from commit aa272e9)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove code deprecated in 0.21 (released on June 30, 2022)
6 participants