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

Upgrade EstimatorResult dataclass #8105

Closed
wants to merge 9 commits into from

Conversation

pedrorrivero
Copy link
Member

@pedrorrivero pedrorrivero commented May 24, 2022

Summary

Upgrades the EstimatorResult dataclass.

Breaking change*
Closes #8100
Changelog: New Feature

Details and comments

  • Rename values field to expectation_values.
  • Add variances field, pulling it out of metadata.
  • Remove numpy array type in favor of core python tuple.
  • Change metadata list type (mutable) in favor of tuple (immutable).

*Assuming that these objects are meant to be consumed by the users, not instantiated, the only breaking change is the metadata type change from list to tuple; since we can alias expectation_values to the old name values with the appropriate type casting.

To do

  • Update Estimator class.
  • Fix and update tests.
  • Add release note.

@pedrorrivero pedrorrivero requested review from a team, ikkoham and t-imamichi as code owners May 24, 2022 16:17
@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 the following people are requested to review this:

@pedrorrivero pedrorrivero marked this pull request as draft May 24, 2022 16:17
@pedrorrivero pedrorrivero changed the title Rename values to expvals, add variances, and update types Upgrade EstimatorResult dataclass May 24, 2022
@jakelishman
Copy link
Member

I'm not involved in this module at all, so my view isn't especially important, I just wanted to point out that making breaking changes to an API is generally not something we allow within Qiskit. We have a complete deprecation policy, but extensible APIs that other people are meant to implement can't really be changed without some form of versioning, because there's not usually a way to make things valid for two different versions of Terra at the same time.

But do note that I'm not involved with primitives at all, so I'm not a reviewer for this change, and welcome to the org!

@t-imamichi
Copy link
Member

We put variance in metadata because we have not concluded which information to add in addition to the expectation value, variance, standard deviation or standard error. We currently use metadata because we can modify it without changing the interface. If we put the additional information as the attribute, we have to choose which to include, variance, standard deviation or standard error.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2385783224

  • 13 of 13 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 84.38%

Totals Coverage Status
Change from base Build 2371064921: 0.008%
Covered Lines: 54548
Relevant Lines: 64646

💛 - Coveralls

@pedrorrivero
Copy link
Member Author

Has any decision been reached already @t-imamichi ?

@t-imamichi
Copy link
Member

I'm afraid that we don't have discussion about metadata yet.

@pedrorrivero
Copy link
Member Author

Outdated, closing and starting anew.

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.

Upgrade primitive result dataclasses
5 participants