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

Fix result.to_dict() following new 2.0 typing #14124

Merged
merged 6 commits into from
Mar 31, 2025

Conversation

valente
Copy link
Contributor

@valente valente commented Mar 28, 2025

Following on the changes in #13793, I think we should also stop trying to serialize into a dictionary since the header should already be a dictionary.

Currently we face the following issue when doing result.to_dict()

  File "/opt/app-root/lib64/python3.11/site-packages/qiskit/result/result.py", line 91, in to_dict
    "header": None if self.header is None else self.header.to_dict(),
                                               ^^^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute 'to_dict'

Added corresponding regression test based on existing to_repr test

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Mar 28, 2025
@CLAassistant
Copy link

CLAassistant commented Mar 28, 2025

CLA assistant check
All committers have signed the CLA.

@valente valente marked this pull request as ready for review March 28, 2025 14:37
@valente valente requested a review from a team as a code owner March 28, 2025 14:37
@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

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.

Thanks for the fix! This must have been an oversight from the qobj removal. To make sure it doesn't slip into the cracks again, could you please add a unit test for the fix? We'd also need a release note.

@coveralls
Copy link

coveralls commented Mar 28, 2025

Pull Request Test Coverage Report for Build 14167128266

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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 114 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.02%) to 88.082%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 94.79%
crates/qasm2/src/lex.rs 3 92.48%
crates/cext/src/exit_codes.rs 7 0.0%
crates/cext/src/sparse_observable.rs 103 0.0%
Totals Coverage Status
Change from base Build 14129997569: 0.02%
Covered Lines: 72754
Relevant Lines: 82598

💛 - Coveralls

@ElePT ElePT added this to the 2.0.0 milestone Mar 31, 2025
@ElePT ElePT added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Mar 31, 2025
@ElePT ElePT changed the title [WIP] fix result.to_dict() following new 2.0 typing Fix result.to_dict() following new 2.0 typing Mar 31, 2025
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.

Thanks a lot again @valente for catching this oversight. I pushed a few changes directly to the branch because of time pressure, as the release is planned for today and this fix unblocks the migration of internal packages to 2.0. I realized that we actually didn't need the release note, as the original PR is technically not released. I also added explicit typing for the remaining input arguments of Result to try to avoid future ambiguities. I am approving but we cannot merge the PR until the CLA is signed. Would you mind clicking on the CLA comment above to get it signed asap?

@ElePT ElePT added stable backport potential The bug might be minimal and/or import enough to be port to stable and removed Changelog: Bugfix Include in the "Fixed" section of the changelog labels Mar 31, 2025
@ElePT ElePT added this pull request to the merge queue Mar 31, 2025
@ElePT ElePT added the Changelog: None Do not include in changelog label Mar 31, 2025
Merged via the queue into Qiskit:main with commit 00c4756 Mar 31, 2025
21 checks passed
mergify bot pushed a commit that referenced this pull request Mar 31, 2025
* fix result.to_dict() following new 2.0 typing

* Update qiskit/result/result.py

Co-authored-by: Luciano Bello <[email protected]>

* add result to_dict test

* add release note

* Update type hints to reflect current use of Result. We no longer have any ExperimentHeader class to call  on, so the type can only be dict.

* Remove reno, as the bug is technically unreleased

---------

Co-authored-by: valente <[email protected]>
Co-authored-by: Luciano Bello <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
(cherry picked from commit 00c4756)
github-merge-queue bot pushed a commit that referenced this pull request Mar 31, 2025
* fix result.to_dict() following new 2.0 typing

* Update qiskit/result/result.py

Co-authored-by: Luciano Bello <[email protected]>

* add result to_dict test

* add release note

* Update type hints to reflect current use of Result. We no longer have any ExperimentHeader class to call  on, so the type can only be dict.

* Remove reno, as the bug is technically unreleased

---------

Co-authored-by: valente <[email protected]>
Co-authored-by: Luciano Bello <[email protected]>
Co-authored-by: Elena Peña Tapia <[email protected]>
(cherry picked from commit 00c4756)

Co-authored-by: Abel Valente <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants