-
Notifications
You must be signed in to change notification settings - Fork 205
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
Implements a reverse_map() method for JordanWignerMapper(). #1344
base: main
Are you sure you want to change the base?
Conversation
|
I see there are failures which are not of this change but due to a newly released pylint version - I created #1346 as this needs fixing outside of this PR to get CI running again and passing on the code that is already in main here. |
Sure, I'll fix the mypy error by pushing the complex inside FermionicOp. |
Included the pylint fixes in this PR. Hope that's'okay. Should fix #1346 |
Would it be too much to ask if you could do them in a separate PR. I think we will need to get the same fixes onto the stable branch so would want to backport the PR. Given this PR introduces a new feature its not something we would backport - that is more for critical fixes where we can do a bug fix release to address things. Of course once such a separate PR was merged, and the branch here updated, it would all pass. |
I'll let this run so we can check things anyway if you like. (Once you have a PR that has been merged here it will not longer need a maintainers approval to run - its just on the first PR) |
be27527
to
4c15ca7
Compare
Pull Request Test Coverage Report for Build 8665226229Details
💛 - Coveralls |
releasenotes/notes/reverse-jordan-wigner-mapping-be0e0ab217967f61.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/reverse-jordan-wigner-mapping-be0e0ab217967f61.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/reverse-jordan-wigner-mapping-be0e0ab217967f61.yaml
Outdated
Show resolved
Hide resolved
return total_fermionic_op.normal_order() | ||
|
||
@staticmethod | ||
def invert_pauli_terms(sparse_pauli_op: SparsePauliOp) -> SparsePauliOp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking this should be private method - ie something we do not want as part of supported public API. (If it was it would havte to have full docstring stating args, return etc. I think this is more intended as some private utility right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now I think I’m the only one asking for this. I don’t see why we shouldn’t have this on the public API though. If Open Fermion can do this and thinks it’s relevant why shouldn’t Qiskit Nature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are talking just about the invert_pauli_terms
method right - that was all I was meaning as being private not the whole reverse map method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! my bad. Sure, I can turn that into a private method. It's just a utility for the reverse_map().
|
||
@classmethod | ||
def reverse_map(cls, qubit_op: SparsePauliOp) -> FermionicOp: | ||
"""Maps a qubit operator ``SparsePauliOp`` back into the second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question around this. This seems to take qubit operator and maps it back regardless of what it really is. I get if I this is given a JW mapped Fermionic Op as a qubit operator it will reverse that. What happens to some other operator - does it always work and produce something even if its meaningless? Is there any check possible? Should we add any note here about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will work for any operator that is a SparsePauliOp and write that in terms of fermionic operators. But if a user tries to reverse an operator that was mapped with a different mapper(I.e. ParityMapper()) they will just get different ones.
It can also take any qubit operator and turn into a fermionic Op even if it doesn’t make sense, so it will always give a result.
Maybe a note making it clear that this works for JW? I wouldn’t know how to check if an qubit operator was transformed via Jordan Wigner or something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note sounds like the best way then - that says it users responsibility to provide an operator that was mapped with JW to get a meaningful outcome as this does the reverse mapping for any operator on the given assumption it was done with a JW mapping in the first place - or however you want to phrase it,.
…f61.yaml Co-authored-by: Steve Wood <[email protected]>
…f61.yaml Co-authored-by: Steve Wood <[email protected]>
Sorry my bad in the suggestion, if you did not correct it was
should have been
|
@woodsp-ibm Let me know if you think the note is okay when you can. |
Co-authored-by: Steve Wood <[email protected]>
@mrossinek Can I get you to take a look at this too. |
Hey, just wanted an update here. Is there anything I should do about this still? |
The build errors with mypy are fixed by #1362, which is still open. As far as progressing this hopefully @mrossinek can take a look. |
Summary
This implements a reverse_map() method in the JordanWignerMapper(). It allows users to go back from qubit operators into fermionic operators. Completes issue #255.
Also included a unit test.
Details and comments
Instead of using the implementation linked in issue #255 I modified it to use the FermionicOp object for the mapping. Seemed like it would be easier for anyone to understand the code. Both implementations are equivalent.