SummedOp updates & optimization converters to use Opflow by Cryoris · Pull Request #1059 · qiskit-community/qiskit-aqua

Cryoris · 2020-06-18T12:36:15Z

Summary

#1077 should be merged first.

Update the converters in the optimization module to accept and return OperatorBase objects (WeightedPauliOperator is supported as before). Also adds an equals implementation to SummedOp which was necessary for the tests to work and moves simplify into reduce as suggested in #1000 (comment).

~~Marked as on hold to give @t-imamichi time for some performance checks.~~
~~WIP: Need to fix set comparison if SummedOp contains other primitives than just PauliOp.~~\

Details and comments

equals has been updated such that expressions such as X + X == 2*X and X + Z == Z + X evaluate to True, unlike before. This calls reduce on the operators and is is slower than the previous equals used in ListOp I removed two lines of add which checked if other is equal to self, as this adds a lot of overhead e.g. if many Pauli terms are summed. Since the opflow focuses on ease-of-use and intuitive syntax close to math, I think it is very important to have X + X = 2X and XZ = ZX equivalencies.

simplify has been renamed to collapse_summands to be more descriptive and such that users have one distinct method they should call to reduce/simplify the operator, namely reduce.

…to summed-op-compare

leads to problems if the coeff is exponentitated and not supposed to be complex

t-imamichi · 2020-06-22T08:46:20Z

I checked the performance with some Hamiltonians of molecules and confirmed that it works well.
I add a short comment for simplicity.

…to summed-op-compare

* explicitly raise an error upon ListOp input * return identity op instead of the int 0

…to summed-op-compare

adekusar-drl · 2020-07-06T13:14:32Z

@Cryoris seems like this PR causes troubles in the build pipeline.

Cryoris · 2020-07-06T13:54:31Z

Indeed there seems the word summands which is marked as error by the spellcheck. The test failures are coming from Qiskit/qiskit#4622, where the type of Instruction.definition has been changed from a list to a circuit. Thus expressions such as if gate.definition == list are failing now.

From the CI log:

if isinstance(gate, IGate) or (type(gate) == Instruction and gate.definition == []):
      File "/home/travis/miniconda/lib/python3.7/site-packages/qiskit/circuit/quantumcircuit.py", line 211, in __eq__
    return circuit_to_dag(self) == circuit_to_dag(other)

Thanks for the info!

adekusar-drl · 2020-07-06T14:08:54Z

Haven't looked into details but some other PRs(including my own) are failing at

qiskit/aqua/utils/controlled_circuit.py:125:12: E1120: No value for argument 'c' in function call (no-value-for-parameter)

Cryoris · 2020-07-06T14:11:44Z

Not sure why that happens, but in #1100 @manoelmarques also had to change the input to controlled_circuit. Looks like the linter wants to have the arguments unrolled 🤔

adekusar-drl · 2020-07-06T14:22:06Z

Just curious, something in this PR caused this behavior or the problem somewhere else?

Cryoris · 2020-07-06T15:39:12Z

Pretty sure this is from somewhere else, as this PR didn't touch controlled_circuit and the failures of the tests in the CI are coming from Qiskit/qiskit#4622. This PR was able to merge because it was built and tested before that terra PR merged and has not been rebuilt since then.

adekusar-drl · 2020-07-06T15:51:28Z

Thanks for the update, a pretty large PR in terra as I see.

…unity#1059) * simplify and reduce, add equals to SummedOp, update tests * directly use new opflow, no need to go via WPO * update comments and docstrings * directly use opflow * don't do equality check in add * directly use opflow * change order in reduce * fix qaoa * add short test on summed op equality * rm prints * use set comparison, rename simplify to collapse_summands * fix expected value, should be sqrt(2), not 2 * cast coeffs to complex * add reno on equals * fix mypy * fix spell * fix lint * dont cast coefficient to complex leads to problems if the coeff is exponentitated and not supposed to be complex * use sum instead of reduce * rm unused import * move __hash__ to primitive op and base on repr * use != over not == * add summed op test for different primitives * check for opbase, not summedop * adress changes from review * explicitly raise an error upon ListOp input * return identity op instead of the int 0 * fix spell * add note that equals is not mathematically sound Co-authored-by: Manoel Marques <manoel@us.ibm.com>

…unity/qiskit-aqua#1059) * simplify and reduce, add equals to SummedOp, update tests * directly use new opflow, no need to go via WPO * update comments and docstrings * directly use opflow * don't do equality check in add * directly use opflow * change order in reduce * fix qaoa * add short test on summed op equality * rm prints * use set comparison, rename simplify to collapse_summands * fix expected value, should be sqrt(2), not 2 * cast coeffs to complex * add reno on equals * fix mypy * fix spell * fix lint * dont cast coefficient to complex leads to problems if the coeff is exponentitated and not supposed to be complex * use sum instead of reduce * rm unused import * move __hash__ to primitive op and base on repr * use != over not == * add summed op test for different primitives * check for opbase, not summedop * adress changes from review * explicitly raise an error upon ListOp input * return identity op instead of the int 0 * fix spell * add note that equals is not mathematically sound Co-authored-by: Manoel Marques <manoel@us.ibm.com>

Cryoris added 14 commits June 17, 2020 15:00

simplify and reduce, add equals to SummedOp, update tests

f58bee5

directly use new opflow, no need to go via WPO

4b0232f

update comments and docstrings

211aebd

directly use opflow

ffa5020

Merge branch 'master' into summed-op-compare

4d431a8

don't do equality check in add

3e24231

directly use opflow

4c6597d

change order in reduce

951efb9

fix qaoa

c4b3ca7

add short test on summed op equality

6678df1

rm prints

d2865d6

use set comparison, rename simplify to collapse_summands

87f5448

fix expected value, should be sqrt(2), not 2

f34f4ea

cast coeffs to complex

b5bd373

Cryoris added the status: on hold E.g. needs more discussion or more information (preformance, tests, ...) label Jun 18, 2020

Cryoris requested a review from t-imamichi June 18, 2020 12:36

Cryoris requested review from manoelmarques, stefan-woerner and woodsp-ibm as code owners June 18, 2020 12:36

Cryoris added the Changelog: New Feature Include in the Added section of the changelog label Jun 18, 2020

Cryoris and others added 7 commits June 18, 2020 14:38

add reno on equals

a499d0b

Merge branch 'master' into summed-op-compare

d610890

fix mypy

eb8ef07

Merge branch 'summed-op-compare' of github.com:Cryoris/qiskit-aqua in…

898178c

…to summed-op-compare

fix spell

5b77cb6

fix lint

596d796

dont cast coefficient to complex

0fb497e

leads to problems if the coeff is exponentitated and not supposed to be complex

t-imamichi reviewed Jun 22, 2020

View reviewed changes

Comment thread qiskit/aqua/operators/list_ops/summed_op.py Outdated

Merge branch 'master' into summed-op-compare

9f80d9b

Cryoris added 7 commits June 26, 2020 16:33

use != over not ==

42d23b9

Merge branch 'summed-op-compare' of github.com:Cryoris/qiskit-aqua in…

823d6de

…to summed-op-compare

Merge branch 'op-hash' into summed-op-compare

fc0daa4

add summed op test for different primitives

bc03704

check for opbase, not summedop

5a5bf03

Merge branch 'master' into summed-op-compare

7a61f23

adress changes from review

b3f389d

* explicitly raise an error upon ListOp input * return identity op instead of the int 0

Cryoris changed the title ~~[WIP] SummedOp updates & optimization converters to use Opflow~~ SummedOp updates & optimization converters to use Opflow Jun 29, 2020

Cryoris added 5 commits June 29, 2020 15:29

fix spell

01008e1

Merge branch 'master' into summed-op-compare

6c625bf

Merge branch 'master' into summed-op-compare

cbb4429

add note that equals is not mathematically sound

dc8f68c

Merge branch 'summed-op-compare' of github.com:Cryoris/qiskit-aqua in…

8c81e9d

…to summed-op-compare

Cryoris mentioned this pull request Jul 3, 2020

SummedOp: equals should capture sums of matrices #1095

Closed

woodsp-ibm approved these changes Jul 3, 2020

View reviewed changes

Cryoris merged commit 6f9629d into qiskit-community:master Jul 6, 2020

Cryoris deleted the summed-op-compare branch July 6, 2020 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SummedOp updates & optimization converters to use Opflow#1059

SummedOp updates & optimization converters to use Opflow#1059
Cryoris merged 38 commits into
qiskit-community:masterfrom
Cryoris:summed-op-compare

Cryoris commented Jun 18, 2020 •

edited

Loading

Uh oh!

t-imamichi commented Jun 22, 2020

Uh oh!

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Cryoris commented Jul 6, 2020

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Cryoris commented Jul 6, 2020

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Cryoris commented Jul 6, 2020

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Cryoris commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

t-imamichi commented Jun 22, 2020

Uh oh!

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Cryoris commented Jul 6, 2020

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Cryoris commented Jul 6, 2020

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Cryoris commented Jul 6, 2020

Uh oh!

adekusar-drl commented Jul 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cryoris commented Jun 18, 2020 •

edited

Loading