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

Bug fix lumped_capacitive.py #981

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Conversation

vanitery
Copy link
Contributor

What are the issues this pull addresses (issue numbers / links)?

Error when running lumped_capacitive.levels_vs_ng_real_units with do_plot = 1.

Did you add tests to cover your changes (yes/no)?

None. Default setting of the function is do_plots=0, which is the only reason this issue hasn't broken out before. Adjusting this single argument in a plt.plot() call should have no bearing on any other function.

Did you update the documentation accordingly (yes/no)?

No, since this is covered in the documentation of matplotlib.

Did you read the CONTRIBUTING document (yes/no)?

Yes, although this seems like such a tiny change, and most of the guidelines overkill in this case.

Summary

Changing plt.plot() argument from "LineWidth" to "linewidth" to avoid run error. Specifically the function levels_vs_ng_real_units inside the file lumped_capacitive.py.

Details and comments

Tiny fix for an aggravating bug.

Matplotlib does not accept "LineWidth" as an argument, but it does accept "linewidth". Tiny fix for a very aggravating bug.
@CLAassistant
Copy link

CLAassistant commented Jan 10, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@zlatko-minev zlatko-minev left a comment

Choose a reason for hiding this comment

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

Wonderful! Thank you very much for the good catch

@vanitery vanitery closed this Jan 15, 2024
@vanitery vanitery reopened this Jan 15, 2024
@vanitery
Copy link
Contributor Author

Yeah sorry about that my friend, I was confused so I accidentally closed it and then reopened it. This makes re-approval necessary and I apologize for the inconvenience.

I was just wondering what I had to do to implement the change now that you approved the pull request? It was still not merged. I am somewhat new to this unfortunately.

@zlatko-minev
Copy link
Collaborator

No problem at all! I understand the confusion. The reason for the delay in merging is that we have a testing process in place, and I needed to wait for the test to finish running before I can click the final "merge" button. It's a necessary step to ensure that everything is working as expected.

I appreciate your patience in this process. I didn't click it yet though because i was distracted with some other things. You can drop me a comment note to remind me here in the future. Thank you for your understanding, and I'll make sure to proceed with the merge as soon as the testing is complete.

@zlatko-minev
Copy link
Collaborator

For example:
image

@vanitery
Copy link
Contributor Author

Seems good to go again.

@zlatko-minev zlatko-minev merged commit 6122ebf into qiskit-community:main Jan 16, 2024
17 checks passed
@zlatko-minev
Copy link
Collaborator

Done!

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.

3 participants