Skip to content

Conversation

viathor
Copy link
Collaborator

@viathor viathor commented Oct 17, 2018

Fixes #446.

Note that QubitOperator has already been capable of simplifying terms before this PR. However, the old algorithm was implemented in an override for __imul__() and therefore was not executed on initialization. There was also the trivial simplification logic (sorting by index) for operators that commute for different indices which was executed in the two parsing functions.

This PR moves the trivial simplification logic into its own function _simplify() in SymbolicOperator and ensures it is called after initialization and multiplication. It also adds an override in QubitOperator that performs the simplifications described in #446. Finally, it removes the now unnecessary override for __imul__() in QubitOperator.

This affects BosonOperator since it commutes across different indices. Hence the need to update unit tests.

The PR also adds unit tests for the simplification logic.

@viathor
Copy link
Collaborator Author

viathor commented Oct 17, 2018

The three failing tests are

  • ExamplesTest.test_can_run_examples_jupyter_notebooks
  • OpenFermionPubChemTest.test_helium
  • OpenFermionPubChemTest.test_water

defined in

  • src/openfermion/tests/_examples_test.py,
  • src/openfermion/utils/_pubchem_test.py.

All three are due to URLError exception:

E           URLError: <urlopen error [Errno 99] Cannot assign requested address>
/opt/python/2.7.14/lib/python2.7/urllib2.py:1198: URLError

We're trying to connect to pubchem.ncbi.nlm.nih.gov. This looks like a temporary error or a configuration issue. I ran the tests locally a few times and they succeed:

(ofpy2) 16:46:39 viathor@ubik:~/src/OpenFermion$ pytest src/openfermion/tests/_examples_test.py src/openfermion/utils/_pubchem_test.py
============================================================================ test session starts ============================================================================
platform linux2 -- Python 2.7.13, pytest-3.8.2, py-1.7.0, pluggy-0.7.1
rootdir: /usr/local/google/home/viathor/src/OpenFermion, inifile:
collected 5 items                                                                                                                                                           

src/openfermion/tests/_examples_test.py ..                                                                                                                            [ 40%]
src/openfermion/utils/_pubchem_test.py ...                                                                                                                            [100%]

============================================================================= warnings summary ==============================================================================
[...warnings elided...]
-- Docs: https://docs.pytest.org/en/latest/warnings.html
================================================================== 5 passed, 10 warnings in 10.78 seconds ===================================================================
(ofpy2) 16:47:02 viathor@ubik:~/src/OpenFermion$

Triggered a re-run on travis.

@viathor
Copy link
Collaborator Author

viathor commented Oct 18, 2018

One(*) of the three tests failed again, the other two succeeded.

(*) ExamplesTest.test_can_run_examples_jupyter_notebooks

@viathor
Copy link
Collaborator Author

viathor commented Oct 18, 2018

All tests passed on re-run. Filed #481 about the flakiness.

Coverage loss is a single line in QubitOperator's different_indices_commute(). If we want this line covered then we should probably just add a unit test that asserts the property is True. If we do that, then we should probably do this for all related attributes across all operator classes. Perhaps in a separate PR?

Copy link
Contributor

@kevinsung kevinsung 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!

@kevinsung kevinsung merged commit ffa9c50 into quantumlib:master Oct 18, 2018
@viathor viathor deleted the qubit-operator-simplify branch October 18, 2018 22:33
@babbush
Copy link
Contributor

babbush commented Oct 19, 2018

@viathor thanks for this pull request. Note that the issue you were running into with that code calling the pubchem website has come up many times before. If you have any reasonable solutions, perhaps even removing the tests for that completely, it would be quite helpful actually.

@viathor
Copy link
Collaborator Author

viathor commented Oct 19, 2018

OK, grabbed the issue I filed yesterday, #481 and will look into it once I have a moment. I don't think we need to remove the tests, it's probably just a matter of making a local copy of the dataset that is being downloaded.

philipp-q pushed a commit to philipp-q/OpenFermion that referenced this pull request Sep 2, 2020
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.

4 participants