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.

Sign up for GitHub

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

Jump to bottom

feat: support sum(LinearMixin) #8722

Merged
merged 2 commits into from
Sep 9, 2022
Merged

feat: support sum(LinearMixin) #8722

merged 2 commits into from
Sep 9, 2022

Conversation

mrossinek
Copy link
Member

Summary

This enables the use of sum(...) for subclasses of the LinearMixin class. It also adds the reflective operand dunder methods __radd__ and __rsub__.

Details and comments

Since this is not part of the publicly documented API, I did not add a reno.

@mrossinek mrossinek requested review from a team and ikkoham as code owners September 9, 2022 10:36
@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:

jakelishman
jakelishman reviewed Sep 9, 2022
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Mathematics aside, this looks good to me. It wouldn't hurt to have a feature release note saying that "BaseOperator subclasses (like x, y and z) can now be used with sum" - the magic methods aren't in the documentation, but the effects can still be seen by users.

qiskit/quantum_info/operators/mixins/linear.py Outdated Show resolved Hide resolved
@mrossinek mrossinek force-pushed the linear-reflected-operands branch 3 times, most recently from 93a4b2c to bdbadc3 Compare September 9, 2022 11:17
jakelishman
jakelishman reviewed Sep 9, 2022
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Maybe to test __rsub__ we just call the method directly? We can compare a.__sub__(b) and b.__rsub__(a). I think we can trust Python to do the magic delegation correctly, so manually calling seems cleanest. The reason to have more than 0 - a is because that wouldn't have caught the original typo.

@mrossinek
feat: support sum(LinearMixin)
2e02589 
This enables the use of `sum(...)` for subclasses of the `LinearMixin`
class. It also adds the reflective operand dunder methods `__radd__` and
`__rsub__`.
jakelishman
jakelishman approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Great, thanks Max. I pushed a commit to add the Sphinx crossrefs to the release note, and we're good to go.

@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog automerge labels Sep 9, 2022
@mrossinek
Copy link
Member Author

Thanks, Jake, for the speedy review process! 🙏

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3022252832

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 84.307%
Files with Coverage Reduction New Missed Lines %
qiskit/quantum_info/operators/mixins/linear.py 2 90.91%
Totals Coverage Status
Change from base Build 3017902091: 0.004%
Covered Lines: 57826
Relevant Lines: 68590
💛 - Coveralls

@mergify mergify bot merged commit e8001f5 into Qiskit:main Sep 9, 2022
@mrossinek mrossinek deleted the linear-reflected-operands branch September 9, 2022 13:16
@ikkoham
Copy link
Contributor

ikkoham commented Sep 9, 2022

LGTM (after merged haha)

@jakelishman
Copy link
Member

Sorry - I thought it'd be inconsequential enough not to need to worry you about it! (And I didn't expect you to be at work at 23:00...)

manoelmarques pushed a commit to manoelmarques/qiskit-terra that referenced this pull request Sep 12, 2022
@mrossinek @jakelishman @manoelmarques
feat: support sum(LinearMixin) (Qiskit#8722)
94c3ab3 
* feat: support sum(LinearMixin)

This enables the use of `sum(...)` for subclasses of the `LinearMixin`
class. It also adds the reflective operand dunder methods `__radd__` and
`__rsub__`.

* Add crossreferences to release note

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
mergify bot added a commit that referenced this pull request Sep 24, 2022
@manoelmarques @Cryoris
@1ucian0 @jakelishman @mrossinek @woodsp-ibm @mergify
Rewrite Amplitude Estimators with Primitives (#8656)
ae0ceee 
* Rewrite Amplitude Estimators with Primitives

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* fix errors

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/fae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/fae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/iae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* fix annotations

* Add sampler properties

* refactor evaluate_measurements

* deprecate qiskit.pulse.utils.deprecated_functionality in favor of qiskit.utils.deprecation.deprecate_function (#8696)

* deprecate deprecated_functionality

* pylint: disable=cyclic-import

* Add reno

* Include Terra version in deprecation notice

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* feat: support sum(LinearMixin) (#8722)

* feat: support sum(LinearMixin)

This enables the use of `sum(...)` for subclasses of the `LinearMixin`
class. It also adds the reflective operand dunder methods `__radd__` and
`__rsub__`.

* Add crossreferences to release note

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* set shots on fae sampler

* cache circuits during estimate

* Change algos estimate

* Change from run_options ro options

* remove circuits cache list

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/ae_utils.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* change slice

* Get shots from metadata

* Update qiskit/algorithms/amplitude_estimators/iae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Rearrange annotation None

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Fix deprecation msg

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: dlasecki <dal@zurich.ibm.com>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
@manoelmarques @Cryoris
@1ucian0 @jakelishman @mrossinek @woodsp-ibm @mergify
Rewrite Amplitude Estimators with Primitives (Qiskit#8656)
0cad507 
* Rewrite Amplitude Estimators with Primitives

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* fix errors

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/fae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/fae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/iae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* fix annotations

* Add sampler properties

* refactor evaluate_measurements

* deprecate qiskit.pulse.utils.deprecated_functionality in favor of qiskit.utils.deprecation.deprecate_function (Qiskit#8696)

* deprecate deprecated_functionality

* pylint: disable=cyclic-import

* Add reno

* Include Terra version in deprecation notice

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* feat: support sum(LinearMixin) (Qiskit#8722)

* feat: support sum(LinearMixin)

This enables the use of `sum(...)` for subclasses of the `LinearMixin`
class. It also adds the reflective operand dunder methods `__radd__` and
`__rsub__`.

* Add crossreferences to release note

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* set shots on fae sampler

* cache circuits during estimate

* Change algos estimate

* Change from run_options ro options

* remove circuits cache list

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/ae_utils.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* change slice

* Get shots from metadata

* Update qiskit/algorithms/amplitude_estimators/iae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Rearrange annotation None

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Fix deprecation msg

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: dlasecki <dal@zurich.ibm.com>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.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
@manoelmarques @Cryoris
@1ucian0 @jakelishman @mrossinek @woodsp-ibm @mergify
Rewrite Amplitude Estimators with Primitives (Qiskit/qiskit#8656)
0ee1f8b 
* Rewrite Amplitude Estimators with Primitives

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* fix errors

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/fae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/fae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/iae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* fix annotations

* Add sampler properties

* refactor evaluate_measurements

* deprecate qiskit.pulse.utils.deprecated_functionality in favor of qiskit.utils.deprecation.deprecate_function (Qiskit/qiskit#8696)

* deprecate deprecated_functionality

* pylint: disable=cyclic-import

* Add reno

* Include Terra version in deprecation notice

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* feat: support sum(LinearMixin) (Qiskit/qiskit#8722)

* feat: support sum(LinearMixin)

This enables the use of `sum(...)` for subclasses of the `LinearMixin`
class. It also adds the reflective operand dunder methods `__radd__` and
`__rsub__`.

* Add crossreferences to release note

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* set shots on fae sampler

* cache circuits during estimate

* Change algos estimate

* Change from run_options ro options

* remove circuits cache list

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Update qiskit/algorithms/amplitude_estimators/ae_utils.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* change slice

* Get shots from metadata

* Update qiskit/algorithms/amplitude_estimators/iae.py

Co-authored-by: dlasecki <dal@zurich.ibm.com>

* Rearrange annotation None

* Update qiskit/algorithms/amplitude_estimators/ae.py

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Fix deprecation msg

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: dlasecki <dal@zurich.ibm.com>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.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
Reviewers

@jakelishman jakelishman jakelishman approved these changes

@ikkoham ikkoham Awaiting requested review from ikkoham

Assignees
No one assigned
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mrossinek @qiskit-bot @coveralls @ikkoham @jakelishman