quantumlib / OpenFermion

The electronic structure package for quantum computers.
Apache License 2.0
1.52k stars 376 forks source link

Flatten ops module? #649

Closed kevinsung closed 4 years ago

kevinsung commented 4 years ago

It looks like I was late to comment on #616 so I figured I'd open a new issue to say that I disagree with the "operators"/"representations" dichotomy. At the very least, the naming is quite confusing, because a FermionOperator is a "representation" of a molecular Hamiltonian in the same sense that an InteractionOperator is a "representation". Both can represent the same operator, albeit using different underlying data structures. Besides, not everything in operators inherits from SymbolicOperator (e.g. MajoranaOperator) and not everything in representations inherits from PolynomialTensor (e.g. DiagonalCoulombHamiltonian), further muddying the waters. So I think we should flatten the module, at least until we come up with a more sensible distinction. @ncrubin @obriente

obriente commented 4 years ago

I agree that the naming is a bit confusing at first glance, but I think the distinction is clear. All the objects in 'operators' use a dictionary/hash table structure to store data, but all the operators in 'representations' store data in array form. The former allows for greater flexibility / representation power, but the latter allows for significantly faster manipulation (but is somewhat restricted in what one can represent easily).

I think this distinction between performance and representability is an important one to have going forward (especially as I'd hope one day to look at pushing much of representations into C for further increased performance), so I'd be inclined to keep the submodules. Not so sure about the names though. I had originally suggests 'ops' and 'sparseops', or 'ops' and 'sparse', but I do think that 'representations' is perhaps better than 'sparse' (the idea being that all you're actually storing is an array representation of the operator, the meaning is something you have to implant --- whereas the keys to the hash table contain this meaning to a large extent). I've never been particularly sold on any of them though, but I've also not found any good alternatives.

ncrubin commented 4 years ago

I disagree that a FermionOperator is a representation of a MolecularHamiltonian. A molecular Hamiltonian is a particular element of a Fermionic algebra--i.e. things that can be built with FermionOperator. MolecularHamiltonian contains information about the basis--integrals, mo_coeffs, that inform what the Hamiltonian will be but are not yet tied to ladder operators. To tie them to ladder operators we need to go to spin-orbitals. InteractionOperator contains this information which is an index map from spin-orbital index to coefficient. The indexing in InteractionOperator corresponds to a particular ladder operator. Therefore it encodes an efficient representation (for basis rotations and other things) of the Hamiltonian.

You are right that not everything in operators inherits from Symbolic. The thinking here was these are base algebra objects for different types of algebras.

Same argument for representations. PolynomialTensor was an efficient way to store a particular algebra element (that is the sum of a bunch of algebra "monomials"). The idea of things in representation is that objects that do the job of storage and mapping live here. Thus DiagonalCoulombHamiltonian serves this purpose for a particular collection of fermionic operators.

As for naming, we're always open to suggestions. representations seemed to fit...but I agree there is potential for improvement.

obriente commented 4 years ago

Looking at the definition of 'representation' in math - https://en.wikipedia.org/wiki/Representation_theory - a representation is a map from an abstract algebra to a set of matrices on a vector space. This maps pretty well to the current distinction, so on retrospection I'm pretty happy with the current names. Could go with 'abstract' vs 'representation' if we wanted to make the distinction clear, perhaps?

kevinsung commented 4 years ago

By FermionOperators and InteractionOperators being representations "in the same sense" I simply meant that

FermionOperator('[0^ 1] + [1^ 0]')

and the InteractionOperator with one-body tensor equal to

[[0.0, 1.0]
 [1.0, 0.0]]

both represent the exact same operator, a_0^\dagger a_1 + a_1^\dagger a_0.

If the distinction is that one class of operators is represented using arrays, then I think we can come up with a better name; I don't find the analogy with representation theory very compelling, since these arrays aren't matrices in the usual sense (they can have more than two dimensions, and we don't think of the arrays themselves as linear maps, rather, the represented operator is the linear map). Also note that "sparse ops" would not be accurate to describe the array representations, since the symbolic representations are actually more sparse (the array representations store a bunch of empty array entries).

ncrubin commented 4 years ago

You are right they represent the same object. That is my starting point as well. The InterationOperator representation you have shown imbues a matrix indexing to represent a particular set of FermionOperators. This format allows for transformations on the operators that get mapped to linear algebra operations on the InteractionOperator or InteractionRDM matrix--i.e wedge, basis transformation, contraction, etc. you can do the same thing in FermionOperator but the code would be inefficient and you would probably end up implementing an InteractionRDM or InteractionOperator! In my head I call these things containers and representations. You can formally evaluate the energy of a Hamiltonian if I give you a bunch of fermionOperators and another list of the expected values. But why not have a container that naturally represents these ideas while leaving the algebra manipulation component (which is simpler with FermionOperator and not uniquely defined with InteractionOperator) to the SymbolicOp like objects? I didn't want to use the name containers since that is also a loaded term. We are open to suggestions on the names of course.

ncrubin commented 4 years ago

@kevinsung did you have further thoughts on this?

kevinsung commented 4 years ago

As you agreed, a FermionOperator and an InteractionOperator can both represent the exact same operator, with the only difference being in the underlying data structure. If we are to separate these classes, we should use names that reflect the difference. After all, both are "operators" and both are "representations," so "operator"/"representation" doesn't make sense to me. Perhaps we could use the names "dict" and "array" instead.

ncrubin commented 4 years ago

I disagree with the conclusion you come to. FermionOperator is a generalization of InteractionOperator so I still think the name representation is okay. You can do some things to InteractionOperator that you can't to FermionOperator. That's the price you pay for generalization/abstraction. "dict" and "array" seem worse, to me, since those are key words and don't carry any meaning with respect to quantum mechanics.

kevinsung commented 4 years ago

"dict" and "array" seem worse, to me, since those are key words and don't carry any meaning with respect to quantum mechanics.

In the language of quantum mechanics, an InteractionOperator is an "operator" just as much as a FermionOperator is an "operator." I don't think the word "representation" as it's being used here has a meaning in quantum mechanics.

ncrubin commented 4 years ago

dict is a keyword in python. we can't use that! as is array for numpy which we rely on heavily. Do you have an alternative pair of names to suggest?

obriente commented 4 years ago

How about 'symbolic' and 'sparse'? In the SymbolicOperator and derived classes we are writing down a symbolic form of the operators (quite literally), and I'd say that PolynomialTensor derived classes are supposed to be 2^Nx2^N operators in sparse representations that allow for fast calculations (in the same way that the sparse libraries in scipy allow for faster linear algebra).

kevinsung commented 4 years ago

@obriente As I pointed out above, the symbolic representations are actually sparser than the array representations, since the arrays contain many zero entries. These arrays are dense Numpy arrays, not Scipy sparse matrices. So if anything, the symbolic representations should be called "sparse."

kevinsung commented 4 years ago

Maybe we could call them "symbolic" and "dense."

kevinsung commented 4 years ago

Or maybe just "sparse" (referring to the symbolic operators) and "dense" (for the dense array operators).

obriente commented 4 years ago

There's a pretty strong connection in my mind between 'sparse' and 'optimized for fast calculation', so I would find this definition very confusing. I also definitely wouldn't say 'dense' for the PolynomialTensor class - they're not 2^Nx2^N after all.

How about 'symbolic' and 'tensor' ? That's pretty unambiguous, no?

kevinsung commented 4 years ago

I'm okay with "symbolic" and "tensor".

ncrubin commented 4 years ago

closing since inactive