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

Dispatch to old Jordan-Wigner function from new one #4253

Merged
merged 15 commits into from Jun 15, 2023
Merged

Conversation

lillian542
Copy link
Contributor

@lillian542 lillian542 commented Jun 14, 2023

Context:
There are now two functions called jordan_wigner in PennyLane. The existing jordan_wigner function is available as qml.qchem.jordan_wigner but not imported at top level. The new one lives in qml.fermi.jordan_wigner. We want to only have one, imported at top level, but we don't want to break anything for people using the current one in the next PL release, as there is no time for a deprecation cycle.

Additionally, we don't have time to update the many functions that use jordan_wigner to follow the new standard for before the release, so we can't add a deprecation warning yet; this would cause other unrelated functions to raise deprecation warnings through no fault of the user.

Description of the Change:
The new jordan_wigner function uses single dispatch to call one of 3 functions, depending on whether the fermi_operator passed to it is a FermiWord, FermiSentence or list (the legacy version). The new jordan_wigner function thus encompasses all behaviour of the old one (renamed _jordan_wigner_legacy).

The imports have been updated so that the new jordan_wigner function can be imported as qml.jordan_wigner, qml.fermi.jordan_wigner, and qml.qchem.jordan_wigner.

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

@lillian542 lillian542 changed the title Dispatch to old Jordan-Wigner function from new one [WIP] Dispatch to old Jordan-Wigner function from new one Jun 14, 2023
@lillian542 lillian542 marked this pull request as ready for review June 14, 2023 19:30
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #4253 (cd7bfa0) into master (58bdaea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4253   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files         352      352           
  Lines       31754    31759    +5     
=======================================
+ Hits        31686    31692    +6     
+ Misses         68       67    -1     
Impacted Files Coverage Δ
pennylane/__init__.py 100.00% <100.00%> (ø)
pennylane/fermi/conversion.py 100.00% <100.00%> (ø)
pennylane/qchem/observable_hf.py 100.00% <100.00%> (+1.06%) ⬆️

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

image
Why do we have 4 options and not 2?

@lillian542
Copy link
Contributor Author

Why do we have 4 options and not 2?

One is the top-level function, one is FermiWord, one is FermiSentence, one is the old function

@soranjh
Copy link
Contributor

soranjh commented Jun 15, 2023

Possible Drawbacks:
The documentation for the old version is less accessible; the docstring for the new jordan_wigner doesn't mention it or explain its input format.

@soranjh
Copy link
Contributor

soranjh commented Jun 15, 2023

we could temporarily make the docstring for the existing jordan_wigner function longer and have it describe all 3 possible scenarios.

The current docstrings are fine. The old jordan_wigner is just used internally in qchem.

Copy link
Contributor

@soranjh soranjh left a comment

Choose a reason for hiding this comment

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

Thanks @lillian542, looks good. Any idea why codecov complains about #L210?

pennylane/fermi/conversion.py Show resolved Hide resolved
pennylane/fermi/conversion.py Outdated Show resolved Hide resolved
@trbromley
Copy link
Contributor

Why do we have 4 options and not 2?

One is the top-level function, one is FermiWord, one is FermiSentence, one is the old function

Thanks! But why do we have two different signatures for the first 3 options? Also, why do we need ps?

@lillian542
Copy link
Contributor Author

Thanks! But why do we have two different signatures for the first 3 options? Also, why do we need ps?

I'll update them to all take fermionic_op as the name of the first argument, so it's more similar. The overall function has to take fermionic_op, **kwargs because there are different kwargs for the new implementation and the old. Once we deprecate and remove the old version, the 3 remaining can all have the same signature.

We need the ps kwarg for if we want to convert to a PauliSentence instead of Operator. I think @soranjh or @Jaybsoni can say more specifically when we would want to use this functionality.

@soranjh
Copy link
Contributor

soranjh commented Jun 15, 2023

Also, why do we need ps?

Working with ps directly and not converting to operation makes some of the tapering functions more efficient.

Copy link
Contributor

@soranjh soranjh left a comment

Choose a reason for hiding this comment

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

Thanks @lillian542, performed some local tests, all seems good.

@lillian542 lillian542 changed the title [WIP] Dispatch to old Jordan-Wigner function from new one Dispatch to old Jordan-Wigner function from new one Jun 15, 2023
@Jaybsoni
Copy link
Contributor

image
Why do we have 4 options and not 2?

This issue can be fixed by single dispatching over a private function and wrapping it with the main function:

def jordan_wigner(fermi_operator, **kwargs):
    """The main doc string"""
    return _dispatched_jordan_wigner(fermi_operator, **kwargs)

@singledispatch
def _dispatched_jordan_wigner(fermi_operator, *kwargs):
    raise ValueError(f" ... ") 

@ _dispatched_jordan_wigner.register
def _(fermi_operator: FermiWord, ps=False, wire_map=None):
    ....

@Jaybsoni
Copy link
Contributor

image
Why do we have 4 options and not 2?

This issue can be fixed by single dispatching over a private function and wrapping it with the main function:

def jordan_wigner(fermi_operator, **kwargs):
    """The main doc string"""
    return _dispatched_jordan_wigner(fermi_operator, **kwargs)

@singledispatch
def _dispatched_jordan_wigner(fermi_operator, *kwargs):
    raise ValueError(f" ... ") 

@ _dispatched_jordan_wigner.register
def _(fermi_operator: FermiWord, ps=False, wire_map=None):
    ....

Spoke with @soranjh offline, we concluded that this can be left as is in this PR and then separately addressed in the documentation PR.

@lillian542 lillian542 merged commit ef0e1ec into master Jun 15, 2023
43 checks passed
@lillian542 lillian542 deleted the jw-dispatch branch June 15, 2023 21:15
frederikwilde pushed a commit that referenced this pull request Jul 5, 2023
* pylint errors

* update changelog

* tidy up

* hopefully fix formatting complaints

* hopefully fix formatting complaints

* Update doc/releases/changelog-dev.md

* formatting and codecov

* leave original jordan_wigner docstring in place so qchem documentation is unchanged

* Update pennylane/fermi/conversion.py

* fix docstring issue with dispatch

* Revert "fix docstring issue with dispatch"

This reverts commit 9f3e736.
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.

None yet

4 participants