Skip to content
This repository was archived by the owner on Dec 7, 2021. It is now read-only.

Separate simplification of operators from SummedOp.add and fix bugs of add#1000

Merged
manoelmarques merged 14 commits into
qiskit-community:masterfrom
t-imamichi:summedop
May 21, 2020
Merged

Separate simplification of operators from SummedOp.add and fix bugs of add#1000
manoelmarques merged 14 commits into
qiskit-community:masterfrom
t-imamichi:summedop

Conversation

@t-imamichi
Copy link
Copy Markdown
Contributor

@t-imamichi t-imamichi commented May 19, 2020

Summary

This PR separates same operator check in SummedOp.add and makes another method SummedOp.simplify to summing up same operators.
It also fixes all bugs found in #979 and add unit tests.

Fixes #979

minor change: remove unnecessary [] in all and any.

Details and comments

It fixes following bugs.

@t-imamichi t-imamichi changed the title Separate simplification of operators from SummedOp.add and fixes bugs Separate simplification of operators from SummedOp.add and fix bugs of add May 19, 2020
Comment thread qiskit/aqua/operators/list_ops/summed_op.py Outdated
Comment thread qiskit/aqua/operators/list_ops/summed_op.py Outdated
@woodsp-ibm woodsp-ibm added the Changelog: Bugfix Include in the Fixed section of the changelog label May 19, 2020
@t-imamichi
Copy link
Copy Markdown
Contributor Author

I also fixed some comments of AbelianGrouper.

Comment thread qiskit/aqua/operators/list_ops/summed_op.py Outdated
Comment thread qiskit/aqua/operators/list_ops/summed_op.py Outdated
Comment thread test/aqua/operators/test_op_construction.py Outdated
@t-imamichi t-imamichi requested a review from woodsp-ibm May 20, 2020 04:56
Comment thread test/aqua/operators/test_op_construction.py Outdated
Comment thread test/aqua/operators/test_op_construction.py Outdated
Comment thread test/aqua/operators/test_abelian_grouper.py Outdated
Comment thread test/aqua/operators/test_abelian_grouper.py Outdated
Comment thread test/aqua/operators/test_abelian_grouper.py
Cryoris
Cryoris previously approved these changes May 20, 2020
Copy link
Copy Markdown
Contributor

@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.

LGTM, thanks for restructuring the tests.

@t-imamichi
Copy link
Copy Markdown
Contributor Author

Thank you for your feedback!

@Cryoris
Copy link
Copy Markdown
Contributor

Cryoris commented May 20, 2020

The spellchecker is complaining about the word subops, if you add it to the .pylintdict the CI should succeed.

@t-imamichi
Copy link
Copy Markdown
Contributor Author

I changed the comment to be clear.

@woodsp-ibm
Copy link
Copy Markdown
Member

If you looked in the build output then the spell checker did provide alternatives like sub-ops. In general if it can be spelled out or changed its better than adding something like subops, which is not really a word, to the custom dict. So your change was the right thing in my book. Sometimes though its unavoidable as you need to refer to something defined elsewhere.

@t-imamichi
Copy link
Copy Markdown
Contributor Author

Travis failed on Test Aqua 1 Python 3.8 due to the following error.

{1} test.aqua.operators.legacy.test_tpb_grouped_weighted_pauli_operator.TestTPBGroupedWeightedPauliOperator.test_evaluate_qasm_mode [2.386731s] ... FAILED
Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):
      File "/home/travis/build/Qiskit/qiskit-aqua/test/aqua/operators/legacy/test_tpb_grouped_weighted_pauli_operator.py", line 148, in test_evaluate_qasm_mode
    self.assertGreaterEqual(reference[0].real,
      File "/opt/python/3.8.0/lib/python3.8/unittest/case.py", line 1316, in assertGreaterEqual
    self.fail(self._formatMessage(msg, standardMsg))
      File "/opt/python/3.8.0/lib/python3.8/unittest/case.py", line 753, in fail
    raise self.failureException(msg)
    AssertionError: 1.736805965121753 not greater than or equal to 1.7490732614333155

@t-imamichi
Copy link
Copy Markdown
Contributor Author

I'm wondering why results varied even if the seed of simulator is fixed.

@manoelmarques manoelmarques merged commit 9378b85 into qiskit-community:master May 21, 2020
@dongreenberg
Copy link
Copy Markdown
Contributor

This is a great addition, but I think it would probably be more natural to put the new simplify functionality inside SummedOp's reduce, rather than create a new function (unless simplify is slow).

@t-imamichi t-imamichi deleted the summedop branch June 17, 2020 04:31
mtreinish pushed a commit to mtreinish/qiskit-core that referenced this pull request Nov 20, 2020
…f add (qiskit-community/qiskit-aqua#1000)

* removed unnecessary "[]"

* separate SummedOp.simplify out from SummedOp.add

* simplify

* revise comments

* add a unit test

* revise comments

* revise test_op_construction and test_abelian_grouper

* revise comments of abelian_grouper

* revise a comment

* use ddt for test of abelian grouper

* revise a comment
manoelmarques pushed a commit to manoelmarques/qiskit-terra that referenced this pull request Dec 2, 2020
…f add (qiskit-community/qiskit-aqua#1000)

* removed unnecessary "[]"

* separate SummedOp.simplify out from SummedOp.add

* simplify

* revise comments

* add a unit test

* revise comments

* revise test_op_construction and test_abelian_grouper

* revise comments of abelian_grouper

* revise a comment

* use ddt for test of abelian grouper

* revise a comment
manoelmarques pushed a commit to manoelmarques/qiskit-terra that referenced this pull request Dec 7, 2020
…f add (qiskit-community/qiskit-aqua#1000)

* removed unnecessary "[]"

* separate SummedOp.simplify out from SummedOp.add

* simplify

* revise comments

* add a unit test

* revise comments

* revise test_op_construction and test_abelian_grouper

* revise comments of abelian_grouper

* revise a comment

* use ddt for test of abelian grouper

* revise a comment
manoelmarques pushed a commit to qiskit-community/qiskit-optimization that referenced this pull request Jan 14, 2021
…f add (qiskit-community/qiskit-aqua#1000)

* removed unnecessary "[]"

* separate SummedOp.simplify out from SummedOp.add

* simplify

* revise comments

* add a unit test

* revise comments

* revise test_op_construction and test_abelian_grouper

* revise comments of abelian_grouper

* revise a comment

* use ddt for test of abelian grouper

* revise a comment
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Changelog: Bugfix Include in the Fixed section of the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SummedOp.add is slow due to linear search and some bugs related to coefficients

5 participants