-
Notifications
You must be signed in to change notification settings - Fork 2.6k
added get_qpy_version to retrieve qpy version #14212
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
Conversation
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 following people are relevant to this code:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @arujjval, thanks for your contribution! When someone develops a new feature we require the following items:
- The feature must be tested, I suggest adding a unit test in the following file:
test/python/qpy/test_circuit_load_from_qpy.py
- If it's a user-facing feature, it must be documented. For this, you should add it to the API docs together with dump and load: https://github.com/Qiskit/qiskit/blob/main/qiskit/qpy/__init__.py#L100
- Finally, you should add a releasenote following the instructions in the contribution guidelines
If you have any question, let us know.
@ElePT Thanks for the help! I have made changes like you asked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise the new function looks good, I just had a suggestion inline on the release notes and the test code. Also it looks like CI is failing on either lint or docs (although the logs have expired).
Pull Request Test Coverage Report for Build 15395920071Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, just a few small comments on how we cn improve the docs a little bit to make the usage a bit more clear.
This commit reworks the internal logic to ensure we're not advancing the file cursor on exiting the function. If we didn't seek back the 7 bytes we read this would be forcing pretty much every conceivable usage of would be using this as a check and then loading the qpy based on the result. This behavior is then added to the docstring and a test is added to assert this behavior is part of the api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM now, but I think since I touched a fair amount of the code now getting someone else to give it final approval for merge would be prudent.
* added get_qpy_version * added test for get_qpy_version * updated test_qpy_versions with different version numbers * resolved lint errors * Apply suggestions from code review * Do not advance cursor on exit This commit reworks the internal logic to ensure we're not advancing the file cursor on exiting the function. If we didn't seek back the 7 bytes we read this would be forcing pretty much every conceivable usage of would be using this as a check and then loading the qpy based on the result. This behavior is then added to the docstring and a test is added to assert this behavior is part of the api. --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
#14201
Summary
To get the qpy version, I have added a new function, "get_qpy_version," according to the suggestion given in the issue.
Comments
I am unsure where to write the tests for this if needed. Is there any guide about writing tests?