Comments (17)
With the change I proposed in #190 it would be made clear in the automatically generated online documentation that FermionOperator and QubitOperator inherit from SymbolicOperator.
from openfermion.
As a smaller part of this - FermionOperator has the static methods identity and zero, but QubitOperator doesn't have these methods.
from openfermion.
Hey, I was thinking of working on this issue.
Reading through the definitions of FermionOperator
and QubitOperator
, I think once this is completed, behavior of compress
should be changed to remove small real parts of complex coefficients, as it does for small imaginary coefficients. At present, it would reduce 1+i*1e-15
to 1
, but leave i+1e-15
unchanged, which I don't see a reason for.
Also, compress may be able to be done in place to save on unnecessary construction of new terms, although it may not be a bottleneck in any common applications, so I'm not sure if it'd be worth it.
Anyway, I'll get started on implementing a SymbolicOperator
base class and making the others inherit from it.
from openfermion.
Awesome! I agree completely with you that compress should also remove the small real components.
If you'd like, email me at my github username @google.com and I can send you some very old code that might be helpful. As I said in comment, in the early days of the code (before this GitRepo), we did have things setup this way. So I do have some old code implementing the SymbolicOperator that also has a bunch of tests that come with it. Of course, a number of things have changed since then but it still might be useful.
from openfermion.
@idk3 What do you think about removing those static methods completely? I think our code would look nicer if there's only one way to initialize the zero and identity operator.
from openfermion.
I brought up that same suggestion once Kevin and I think Ian and Craig pushed back. @idk3 @Strilanc can you remind us why you think they are necessary?
from openfermion.
IMO the static methods are the only definitive way to initialize zero and identity, and FermionOperator()
and FermionOperator(())
should be avoided where possible.
My basic problem is that FermionOperator()
and FermionOperator(())
are ambiguous, to the point that they actually used to do the opposite of what they do now. The static methods make the distinction totally clear, both for code readability (no confusion about whether something is zero or identity) as well as for flexibility for future changes (should the initializer change again, the only thing we have to change is the static method, rather than literally every file in OpenFermion, as I had to do last time the definition changed ;)).
@Strilanc might have other reasons beyond these.
from openfermion.
Hmm I see your concerns @idk3 . I guess what I'm proposing is that we agree here that the meaning of FermionOperator()
and FermionOperator(())
shouldn't be changed in the future. Their current meanings have always made a lot of sense to me, and I'm pretty sure I'd be strongly opposed to any future change to them. In general, initializing a class instance with no arguments means "do the bare minimum amount of work", in this case meaning initialize an empty terms dictionary, while initializing with the empty tuple ()
as an argument implies that the empty tuple is a valid term, and there really is no other sensible choice for representing the constant shift (identity matrix) term.
from openfermion.
Hi guys, quick update on where I'm at. I've changed the QubitOperator
and FermionOperator
classes to inherit from SymbolicOperator
and moved over all functions with identical behavior between them into _symbolic_operator.py
. So far, all tests continue to pass and existing tests give 100% coverage of _symbolic_operator.py
.
Up next will be messing with behavior a bit (adding static methods zero
and identity
to QubitOperator
, changing compress
as discussed above, adding abs
).
After that will be trying to further reduce code duplication (moving identical code blocks into helper functions in SymbolicOperator
etc), linting, writing documentation, and removing redundant test/adding new ones as needed.
Feel free to check out the changes in my fork.
Edit: For now, zero
and identity
have been changed to class methods in SymbolicOperator
. This will give an ugly error when trying to call SymbolicOperator.zero()
or SymbolicOperator.identity()
. I think that this is not an issue unless we plan to expose SymbolicOperator
to users, which I can't see a reason for at the moment.
from openfermion.
This sounds like great progress. The first thing I will mention though is that you might want to check in the SymbolicOperator class once it is essentially "done". After that, you can check in the change that alters the FermionOperator and QubitOperator classes to inherit from the SymbolicOperator class. I suggest doing things this way to keep the pull requests manageable. This is going to be a big one and I anticipate it being rather confusing if you try to merge everything at once. This also gives us a nice forum to provide feedback in the context of the actual code (easier than flipping back and forth to look at your fork).
You asked me an interesting question offline which I would like to repeat and address here since I don't have a great solution but others might have input. You wrote:
If I understand the documentation generation correctly, it basically formats nicely the comments from the code itself, reading which class/function the documentation is referring to from the code itself. So in the current release, QubitOperator and FermionOperator have a lot of overlapping documentation. In my fork, many of the overlapping functions are moved into SymbolicOperator, so I think they will be documented under SymbolicOperator and not appear under FermionOperator and QubitOperator. I think this might be difficult to follow, especially if it isn't clear when constructing Qubit or Fermion operators that they both inherit from SymbolicOperator. I'm not exactly sure how the documentation process works, so I'd like to hear your thoughts.
The ideal solution would be if we had some way for the documentation to inherit as well! But I don't think that can be done. I suppose my recommendation would just be to make it very clear all over the FermionOperator / QubitOperator documentation that those classes inherit a lot of functionality. One should be able to then click a link in the web docs to see the parent class documentation.
from openfermion.
All right, I've submitted 2 separate PRs, one only containing the SymbolicOperator declaration file and another containing only the changes to QubitOperator and FermionOperator. Of course, the latter will fail tests right now, but I wanted to make the code simple to get to. Once the first one is accepted I can update the second one to include the SymbolicOperator declaration.
I did notice a potential issue: in __iadd__
, the QubitOperator and FermionOperator differ in that the QubitOperator class deletes all terms in the summation that are identically zero, while the FermionOperator implementation deletes all terms whose absolute value is below EQ_TOLERANCE
. I initially used the version with EQ_TOLERANCE
, but with EQ_TOLERANCE = 1e-12
, this caused the test JelliumTest.test_coefficients
to fail. This test would pass with a lower tolerance, but a lower tolerance would cause other tests to fail due to assertAlmostEqual
and friends being more strict. I'm not familiar enough with JelliumTest
to tell if this is intended behavior or not, so I'm not sure what the solution should be.
For now, __iadd__
only deletes terms that are identically zero, so the behavior of FermionOperator
will be slightly different. If this is unwanted, I can specialize the implementation of __iadd__
.
from openfermion.
@bgimby nice catch in JelliumTest
! I think that explains an issue I was having on another PR.
from openfermion.
@bgimby I noticed your change to __iadd__
and changed it back in my last PR, without realizing that you did it for this reason. However, on my computer the JelliumTest still passes. Indeed, it must also have passed on the coveralls server because otherwise we couldn't have merged the PR.
from openfermion.
It passes tests because in your merged PR the behavior was overridden by the definition in _qubit_operator.py
.
In the version of PR #192 that removed the QubitOperator
definition and left the SymbolicOperator
definition as it is now, the travis tests failed.
checking out this commit or checking out the PR #192 branch and manually changing the behavior back should reproduce the bug.
from openfermion.
Ah I see. Hmm we'll need to come up with a better solution than fudging with EQ_TOLERANCE
...
from openfermion.
IMO the best solution is to only remove identically zero terms in functions that do not explicitly state that they remove small terms (like compress
).
The user probably has a better idea than us when it is safe to remove or reduce small terms.
So we can either only remove identically zero terms in __iadd__
and friends, or mention in the documentation that small terms will be removed and then mess with EQ_TOLERANCE
in the jellium test.
from openfermion.
Closed by #192
We should perhaps also write something about this in the arXiv paper...
from openfermion.
Related Issues (20)
- Help with one-body and two-body coefficients for orbital removal
- UHF energy with openfermion HOT 1
- scipy > 1.9.3 breaks QuarticFermionicSimulationGate decompose method. HOT 5
- Incorrect Bounds on Trotter Error
- Incorrect formula to calculate required Trotter steps HOT 1
- Resource estimation code not tested as part of the CI
- Should move to black for formatting.
- Why does MajoranaOperator not subclass SymbolicOperator? HOT 1
- Some inconsistencies in molecular single factorization costings HOT 1
- Inconsistencies in the double factorized chemistry resource estimate costing function
- 91 tests fail HOT 7
- Nightly tests are broken HOT 1
- slight modification to function generate_hamiltonian ?
- Operation between MajoranaOperator and numbers? HOT 5
- QuadraticFermionicSimulationGate tests fail with cirq == 1.3.0 HOT 5
- Hubbard model notebook is flaky
- Trotter evolution time may be off by a factor of 2 HOT 2
- 1 test fails HOT 1
- get_sparse_operator fails on non-simplified QubitOperators
- Bad behavior when working with sympy
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from openfermion.