Skip to content

Adds observable evaluator with primitives.#8683

Merged
mergify[bot] merged 25 commits into
Qiskit:mainfrom
dlasecki:observable-eval-primitives
Sep 15, 2022
Merged

Adds observable evaluator with primitives.#8683
mergify[bot] merged 25 commits into
Qiskit:mainfrom
dlasecki:observable-eval-primitives

Conversation

@dlasecki
Copy link
Copy Markdown
Contributor

@dlasecki dlasecki commented Sep 5, 2022

Summary

This PR adds an observable evaluator that uses the Estimator primitive. It will be used by several primitives-enabled algorithms. It replaces the aux_ops_evaulator.py.

  • Release notes.
  • Handle 0 passed in as operators.
  • Deprecation of aux_ops_evaluator.py. @manoelmarques

Closes: #8505

Details and comments

@qiskit-bot
Copy link
Copy Markdown
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:

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 5, 2022

Pull Request Test Coverage Report for Build 3062532045

  • 45 of 58 (77.59%) changed or added relevant lines in 3 files are covered.
  • 177 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.3%) to 84.203%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/algorithms/observables_evaluator.py 42 55 76.36%
Files with Coverage Reduction New Missed Lines %
src/sabre_swap/sabre_dag.rs 1 94.44%
src/optimize_1q_gates.rs 57 4.84%
src/sabre_swap/mod.rs 59 80.45%
src/results/marginalization.rs 60 54.02%
Totals Coverage Status
Change from base Build 3062284929: -0.3%
Covered Lines: 59322
Relevant Lines: 70451

💛 - Coveralls

Copy link
Copy Markdown
Collaborator

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Some comments below otherwise LGTM 👍🏼 now we just need to convince @woodsp-ibm that we need this 😉

Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
@woodsp-ibm
Copy link
Copy Markdown
Member

woodsp-ibm commented Sep 6, 2022

now we just need to convince @woodsp-ibm that we need this

It seems fine - the Dict and List processing to handle None is beyond what the Estimator can handle to the data needs to be mapped in/out and this does that as a common function the algos can use.

Since the Estimator reports variance back, I am wondering whether we should just pass variance on back from here too, when its available. I am not sure about shots being present always to do the conversion and if using Estimator directly I get variance why not also when running an algo that uses it...

Handle 0 and None passed in as operators.

From the main description. While None was used in the lists to preserve order if an operator became invalid as part of transformation down through the stack, 0 is not a valid value. What you might mean by that is empty/zero operator i.e. a valid instance, a zero operator. I would hope Estimator handles that - I know with opflow additional work was done to select these out - which we did back in Aqua days too since the expectation is 0. I do not know what Estimator does.

Comment thread releasenotes/notes/observable-eval-primitives-e1fd989e15c7760c.yaml Outdated
Comment thread releasenotes/notes/observable-eval-primitives-e1fd989e15c7760c.yaml Outdated
@woodsp-ibm
Copy link
Copy Markdown
Member

Any thought on this aspect from above

Since the Estimator reports variance back, I am wondering whether we should just pass variance on back from here too, when its available. I am not sure about shots being present always to do the conversion and if using Estimator directly I get variance why not also when running an algo that uses it...

Copy link
Copy Markdown
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.

I found these small details while reviewing the PVQD PR, and thought it would make more sense to suggest here the changes. Otherwise, I'm using this code on my PR and it looks good to me :)

Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
@manoelmarques manoelmarques force-pushed the observable-eval-primitives branch from 7b75638 to 2bcf07b Compare September 8, 2022 15:05
@dlasecki
Copy link
Copy Markdown
Contributor Author

dlasecki commented Sep 8, 2022

Any thought on this aspect from above

Since the Estimator reports variance back, I am wondering whether we should just pass variance on back from here too, when its available. I am not sure about shots being present always to do the conversion and if using Estimator directly I get variance why not also when running an algo that uses it...

Hello, I have been wondering about it myself. The primitive natively records a variance. I was thinking that if variance is present, shots should always be present, too. Any thoughts on that @t-imamichi?
The old auxiliary ops evaluator returns standard deviations and that's because some algorithms use it. So I decided to preserve that to avoid changes and potential complications in other places. Perhaps a good idea would be to return directly what the primitive reports, i.e. both variance and shots. But this would require some changes in algorithms...

I am fine either way.

@woodsp-ibm
Copy link
Copy Markdown
Member

The old auxiliary ops evaluator returns standard deviations and that's because some algorithms use it.

I was not aware of algorithms using it as such, VQE/VQD/QAOAetc. They did return it as part of their result for anyone to use should they wish, that's the extent of the use I knew, where the aux ops evaluation was a value, std_dev tuple for each.

@ElePT
Copy link
Copy Markdown
Contributor

ElePT commented Sep 9, 2022

The old auxiliary ops evaluator returns standard deviations and that's because some algorithms use it.

I was not aware of algorithms using it as such, VQE/VQD/QAOAetc. They did return it as part of their result for anyone to use should they wish, that's the extent of the use I knew, where the aux ops evaluation was a value, std_dev tuple for each.

Yes, none of these 3 algorithms does actually use these stdevs, they just return them as part of the aux_ops evaluation result. So I think that variance and shots could be returned without much hassle. We would only have to change one type hint in the base class, and the docstring of the aux_ops_result.

@t-imamichi
Copy link
Copy Markdown
Member

As for std_dev, there is a PR #8105 to extend EstimatorResult to include not only variance but also std_dev or anything else. But, we didn't have enough discussion yet.

@declanmillar
Copy link
Copy Markdown
Member

declanmillar commented Sep 9, 2022

Do we want to handle cases where users pass in an empty list or dict for observables?

@dlasecki
Copy link
Copy Markdown
Contributor Author

dlasecki commented Sep 9, 2022

Do we want to handle cases where users pass in an empty list or dict for observables?

Good point! I just fixed 1 line to handle empty inputs such that empty outputs are returned with no error! Added unit tests for that.

Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
@woodsp-ibm woodsp-ibm added this to the 0.22 milestone Sep 12, 2022
Comment thread qiskit/algorithms/aux_ops_evaluator.py
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Comment thread qiskit/algorithms/observables_evaluator.py Outdated
Copy link
Copy Markdown
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

LGTM thanks.

@woodsp-ibm woodsp-ibm added Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog. Changelog: Added Add an "Added" entry in the GitHub Release changelog. automerge labels Sep 15, 2022
@mergify mergify Bot merged commit 50f2eaa into Qiskit:main Sep 15, 2022
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* Added observables_evaluator.py with primitives.

* Added ListOrDict support to observables_evaluator.py.

* Included CR suggestions.

* Applied some CR comments.

* Added reno.

* Support for 0 operator.

* Add pending deprecation

* Code refactoring.

* Code refactoring.

* Improved reno.

* Returning variances and shots.

* Unit test fix.

* Reduced use of opflow.

* Handle empty inputs gracefully.

* Applied CR comments.

* Applied CR comments.

* Eliminated cyclic import.

Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* Added observables_evaluator.py with primitives.

* Added ListOrDict support to observables_evaluator.py.

* Included CR suggestions.

* Applied some CR comments.

* Added reno.

* Support for 0 operator.

* Add pending deprecation

* Code refactoring.

* Code refactoring.

* Improved reno.

* Returning variances and shots.

* Unit test fix.

* Reduced use of opflow.

* Handle empty inputs gracefully.

* Applied CR comments.

* Applied CR comments.

* Eliminated cyclic import.

Co-authored-by: Manoel Marques <manoel.marques@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: Added Add an "Added" entry in the GitHub Release changelog. Changelog: Deprecated Add a "Deprecated" entry in the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rewrite aux_ops_evaluator with primitives.

9 participants