Skip to content

Conversation

arujjval
Copy link
Contributor

@arujjval arujjval commented Apr 16, 2025

#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?

@arujjval arujjval requested a review from a team as a code owner April 16, 2025 05:37
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Apr 16, 2025
@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 following people are relevant to this code:

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

@CLAassistant
Copy link

CLAassistant commented Apr 16, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ElePT ElePT left a 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.

@arujjval
Copy link
Contributor Author

@ElePT Thanks for the help!

I have made changes like you asked.

Copy link
Member

@mtreinish mtreinish left a 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).

@mtreinish mtreinish added Changelog: New Feature Include in the "Added" section of the changelog mod: qpy Related to QPY serialization labels May 8, 2025
@coveralls
Copy link

coveralls commented May 8, 2025

Pull Request Test Coverage Report for Build 15395920071

Warning: 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

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • 39 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.04%) to 87.806%

Files with Coverage Reduction New Missed Lines %
crates/transpiler/src/passes/unitary_synthesis.rs 1 94.57%
crates/circuit/src/symbol_expr.rs 5 75.07%
crates/qasm2/src/lex.rs 9 91.73%
crates/qasm2/src/parse.rs 24 95.76%
Totals Coverage Status
Change from base Build 15393181973: -0.04%
Covered Lines: 80418
Relevant Lines: 91586

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish 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 updates, just a few small comments on how we cn improve the docs a little bit to make the usage a bit more clear.

arujjval and others added 4 commits May 17, 2025 07:46
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.
Copy link
Member

@mtreinish mtreinish left a 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.

@mtreinish mtreinish added this pull request to the merge queue Jun 2, 2025
Merged via the queue into Qiskit:main with commit 6455e8b Jun 2, 2025
26 checks passed
@arujjval arujjval deleted the get-qpy-version branch June 3, 2025 12:42
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: qpy Related to QPY serialization
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants