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.

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

Add openfermion conversion module to qml.qchem #2371

Merged
merged 34 commits into from
Mar 25, 2022

Conversation

soranjh
Copy link
Contributor

@soranjh soranjh commented Mar 23, 2022

Context:
This PR unifies the functions needed for converting openfermion objects to pennylane observables, also see here.

Description of the Change:
The qml.hf.convert module is moved inside qml.qchem and other functions are modified to adapt with this change.

Benefits:
Helps with decreasing external dependencies.

Possible Drawbacks:
NA

Related GitHub Issues:
NA

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@soranjh soranjh marked this pull request as ready for review March 23, 2022 20:02
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #2371 (d5d87b6) into master (d562262) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2371   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files         244      244           
  Lines       18360    18300   -60     
=======================================
- Hits        18254    18195   -59     
+ Misses        106      105    -1     
Impacted Files Coverage Δ
qchem/pennylane_qchem/qchem/__init__.py 100.00% <ø> (ø)
qchem/pennylane_qchem/qchem/hf/__init__.py 100.00% <ø> (ø)
qchem/pennylane_qchem/qchem/convert.py 100.00% <100.00%> (ø)
qchem/pennylane_qchem/qchem/obs.py 100.00% <100.00%> (ø)
qchem/pennylane_qchem/qchem/structure.py 97.38% <100.00%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d562262...d5d87b6. Read the comment docs.

from pennylane import numpy as np
from pennylane import qchem

pytest.importorskip("openfermion")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the order matter for the lines
from openfermion import QubitOperator and pytest.importorskip("openfermion")?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand the pytest docs, I think the pytest.importorskip("openfermion") should come before attempting to import QubitOperator.

Copy link
Contributor Author

@soranjh soranjh Mar 24, 2022

Choose a reason for hiding this comment

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

Modified it to openfermion = pytest.importorskip("openfermion") and then used openfermion.QubitOperator everywhere.

qchem/pennylane_qchem/qchem/convert.py Show resolved Hide resolved
from pennylane import numpy as np
from pennylane import qchem

pytest.importorskip("openfermion")
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand the pytest docs, I think the pytest.importorskip("openfermion") should come before attempting to import QubitOperator.

@obliviateandsurrender
Copy link
Contributor

Looks great @soranjh. Just left a couple of minor comments.

Comment on lines 81 to 82
from pennylane.ops import convert
from pennylane.ops.convert import import_operator
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module convert.py which hosts functions for converting openfermion operator objects to pennylane objects is moved from qml.qchem to qml.ops because of the generality of the module.

openfermion = pytest.importorskip("openfermion")


@pytest.fixture(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from qml.qchem.tests.conftest to here.

@soranjh soranjh added the qchem ⚛️ Related to the QChem package label Mar 24, 2022
@soranjh soranjh changed the title Add openfermion conversion module to qchem Add openfermion conversion module to qml.ops Mar 24, 2022
Copy link
Contributor

@Jaybsoni Jaybsoni left a comment

Choose a reason for hiding this comment

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

Looks good

pennylane/ops/convert.py Outdated Show resolved Hide resolved
qchem/pennylane_qchem/qchem/obs.py Outdated Show resolved Hide resolved
qchem/tests/test_convert_observable.py Show resolved Hide resolved
Copy link
Contributor

@obliviateandsurrender obliviateandsurrender left a comment

Choose a reason for hiding this comment

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

Looks good to go! Just please check the failing codecov test.

@soranjh soranjh merged commit bc912b4 into master Mar 25, 2022
@soranjh soranjh deleted the qchem_openfermion_conversion branch March 25, 2022 19:21
@soranjh soranjh changed the title Add openfermion conversion module to qml.ops Add openfermion conversion module to qml.qchem Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qchem ⚛️ Related to the QChem package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants