Skip to content

Need to remove max_credits#7409

Merged
mergify[bot] merged 9 commits intoQiskit:mainfrom
iuliazidaru:issue4132
Jan 19, 2022
Merged

Need to remove max_credits#7409
mergify[bot] merged 9 commits intoQiskit:mainfrom
iuliazidaru:issue4132

Conversation

@iuliazidaru
Copy link
Copy Markdown
Contributor

@iuliazidaru iuliazidaru commented Dec 14, 2021 •
edited
Loading

Summary

Fix #4132

Details and comments

max_credits was deprecated

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 14, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@iuliazidaru iuliazidaru changed the title Issue4132 Need to remove max_credits Dec 14, 2021
jakelishman
jakelishman reviewed Dec 20, 2021
View reviewed changes
Copy link
Copy Markdown
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 looking at this - this definitely had fallen through the cracks, so it's great you're taking care of it, thank you!

You've done a good job with issuing the deprecations from what I can see, I just left a couple of comments about the message we should emit, and how we can slim down all the tests so it's clearer what we're actually testing.

Comment thread qiskit/assembler/run_config.py Outdated
Comment thread qiskit/assembler/run_config.py
Comment thread test/python/qobj/test_qobj_deprecated.py Outdated
Comment thread test/python/qobj/test_qobj_deprecated.py
@iuliazidaru
Copy link
Copy Markdown
Contributor Author

iuliazidaru commented Dec 21, 2021

@jakelishman Thank you for the review. I fixed the code based on your comments.

jakelishman
jakelishman reviewed Dec 22, 2021
View reviewed changes
Copy link
Copy Markdown
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 the quick turn-around! I spotted one place in qiskit.assemble where we could handle the warning a little better for users, but in general this looks good to me.

After this, I need to check with the team that this parameter is completely deprecated (I'm fairly sure it is) before merging, which might take a little bit of time because everyone's on holiday.

Comment thread qiskit/compiler/assembler.py Outdated
@iuliazidaru
Copy link
Copy Markdown
Contributor Author

iuliazidaru commented Jan 19, 2022

@jakelishman Any news for this PR?

jakelishman
jakelishman approved these changes Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Sorry for the delay, I forgot to check back in after we all got back from christmas! I've just pushed a couple of tweaks to simplify the release note and catch a missing place where max_credits was still in use, but this looks good now, thanks!

@jakelishman jakelishman added automerge Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. labels Jan 19, 2022
@jakelishman jakelishman added this to the 0.20 milestone Jan 19, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 19, 2022

Pull Request Test Coverage Report for Build 1718874755

  • 14 of 15 (93.33%) changed or added relevant lines in 7 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 83.151%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/test/mock/fake_qobj.py 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/utils/run_circuits.py 1 30.79%
Totals Coverage Status
Change from base Build 1714460551: 0.01%
Covered Lines: 51999
Relevant Lines: 62536
💛 - Coveralls

@mergify mergify Bot merged commit 185df89 into Qiskit:main Jan 19, 2022
@1ucian0 1ucian0 mentioned this pull request Dec 26, 2022
Merged
ElePT pushed a commit to ElePT/qiskit-algorithms that referenced this pull request Jul 27, 2023
@iuliazidaru @jakelishman
Need to remove max_credits (Qiskit/qiskit#7409)
bf52abb 
* fix issue Qiskit/qiskit#4132 - Need to remove max_credits

* fix issue Qiskit/qiskit#4132 - Need to remove max_credits

* deprecate max_credits instead of removing

* deprecate max_credits - add release notes

* fix review comments

* add warning for assamble

* Reword release note

* Remove straggler max_credits

Co-authored-by: Jake Lishman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jakelishman jakelishman jakelishman approved these changes

@manoelmarques manoelmarques Awaiting requested review from manoelmarques

@woodsp-ibm woodsp-ibm Awaiting requested review from woodsp-ibm

Assignees

No one assigned

Labels

Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog.

Projects

None yet

Milestone

0.20

Development

Successfully merging this pull request may close these issues.

Need to remove max_credits

4 participants

@iuliazidaru @CLAassistant @coveralls @jakelishman