-
Notifications
You must be signed in to change notification settings - Fork 209
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 MajoranaOp class #1270
Add MajoranaOp class #1270
Conversation
Pull Request Test Coverage Report for Build 8718939695Details
💛 - Coveralls |
I changed |
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.
Thanks for adding this class!
So my question is how would you map a MajoranaOp
to thee qubit space? Are you planning to implement a mapper or perhaps to rely on the Fermioni mappers?
@ftroisi Thanks for your comments!
Yes, the whole point of this exercise is as a means to the end of implementing the ternary tree mapper, cf. #582 (which I have basically completed but need to add some reasonable tests before opening a PR). Regarding the docstrings, as @woodsp-ibm has pointed out, there are generally no docstrings for classes that inherit and can use the parent docstring. I mainly followed 1:1 the implementation of FermionicOp here. I'm not sure if I like this either - I guess it keeps the code compact but needing to find the parent method to understand what a method is supposed to do can be a bit tedious. Nonetheless, I accepted it as a style choice and tried to be consistent. Same goes essentially for the .tensor and .expand methods. Simply trying to be as close to FermionicOp as possible which has both these methods. I have no strong feelings about any of those things, but would argue that if they are changed here one should possibly also discuss them for the other operators and the rest of qiskit-nature. |
The methods are documented in the derived class as well for overridden methods - it takes the docstring from the method in the parent (saves copying the same thing). If you click on the |
@woodsp-ibm |
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.
Alright, I left quite a number of comments but hopefully this gives you everything that you need 👍
Made some changes regarding the more obvious comments. Will work in the rest later this week. Reverted the PR to draft until then. |
Co-authored-by: Max Rossmannek <[email protected]>
Co-authored-by: Max Rossmannek <[email protected]>
That should cover all the points raised now. I hope I didn't forget any. |
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.
Alright, this is looking very good!
I still have some nitpicks and a few more suggestions for unittests but coverage is otherwise very good and I think this is basically good to go.
I did find a semi-related bug (#1298) which we will need to address separately, but this should not hold up the merging of this PR 👍
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.
Relying heavily on my previous reviews, I think this LGTM 👍
Just two very minor things about the release note.
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.
Thank you for this valuable contribution! 🎉
Summary
Adds a new class
.second_q.operators.MajoranaOp
implementing a Majorana fermion operator, along with aM̀ajoranaOp.from_fermionic_op
constructor method to convert aFermionicOp
into aMajoranaOp
. Fixes #1257M̀ajoranaOp
is implemented as aSparseLabelOp
, closely following the implementation ofFermionicOp
. The syntax for operator labels is'_0 _1'
forDetails and comments
Label syntax
The reasoning for choosing$\gamma$ would be, but is problematic as a non-ASCII character, I was considering
'_0 _1'
as a label style was a compromise between compatibility (MajoranaOp
should also be aSparseLabelOp
with a syntax close to those ofFermionicOp
andBosonicOp
), readability, and sparsity. Technically, a Majorana operator is defined by just a list of indices, but any non-string format would be incompatible with the other operators. I was considering something like'$_0 $_1'
with some character $ in order to have the closest possible format to the other operators, but since there is no canonical choice for the $ symbol ('g_0'
,'#_0'
, etc.) I thought, the most reasonable choice is to keep the underscore for readability and clarity that we are dealing with an index but omit the +/-/$/... completely. Since this is clearly a relevant choice and pretty subjective, I post this as a draft pull request and am looking forward to discussing this.Implementation of MajoranaOp
MajoranaOp
is largely copy&paste fromFermionicOp
with some more or less trivial adaptions. The main differences areMajoranaOp.from_fermionic_op
that converts a Fermionic operator to Majorana. Conversion follows the same convention as in openfermion, i.e.num_spin_orbitals
is truly meant to count the number of fermionic orbitals, hence indices run from 0 to2 * num_spin_orbitals - 1
since every orbital with its creation/annihilation operators gets mapped to two Majorana operators.I documented everything mostly in the exact same style as in the docstrings for
FermionicOp
- i.e. I believe that the documentation is taken care off, modulo the issue with arguments I opened in #1269 .I also implemented tests for all methods (again mostly just copy&paste from the corresponding tests of
FermionicOp
).Order of operator classes in documentation
In
__init__.py
I addedMajoranaOp
directly underneathFermionicOp
which makes it the second operator to appear in the list in the API reference beforeBosonicOp
. I guess this is debatable, sinceBosonicOp
is clearly the more important operator, but then I think that it makes sense to group Majorana and Fermionic operators together. Alternatively, one could also putBosonicOp
at the top, followed byFermionicOp
andMajoranaOp
which is the order in which one would most likely find them in a textbook, or simply opt for alphabetical order.Changes to FermionicOp
I made two changes to
FermionicOp
:FermionicOp._index_order
is turned into a classmethod so it can be called from withinMajoranaOp.index_order
as well (to avoid redundancy).FermionicOp.to_matrix
in the docstring since such a method does not seem to exist (unrelated to the Majorana changes)All changes are summarized in the release note.