Skip to content

Need to remove max_credits #7409

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jan 19, 2022
Merged

Need to remove max_credits #7409

merged 9 commits into from
Jan 19, 2022

Conversation

iuliazidaru
Copy link
Contributor

@iuliazidaru iuliazidaru commented Dec 14, 2021 •
edited
Loading

Summary

Fix #4132

Details and comments

max_credits was deprecated

@CLAassistant
Copy link

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
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.

@iuliazidaru
Copy link
Contributor Author

@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
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.

@iuliazidaru
Copy link
Contributor Author

@jakelishman Any news for this PR?

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

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: Deprecation Include in "Deprecated" section of changelog labels Jan 19, 2022
@jakelishman jakelishman added this to the 0.20 milestone Jan 19, 2022
@coveralls
Copy link

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 <jake.lishman@ibm.com>
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: Deprecation Include in "Deprecated" section of 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