ProjectQ-Framework / FermiLib

FermiLib: Open source software for analyzing fermionic quantum simulation algorithms
https://projectq.ch/
Apache License 2.0
87 stars 39 forks source link

Easier data structure access #21

Closed babbush closed 7 years ago

babbush commented 7 years ago

In an earlier version of FermiLib our core data structures supported easier manipulation and access. For instance, the iter method was defined on QubitOperator and FermionOperator so that one could type: for coefficient, terms in my_operator: instead of for term in my_operator.terms: coefficient = my_operator[term]

Also, we had overloaded the set and slice methods for easier access to terms. I found this design to be more intuitive and user-friendly.

Additionally, we need an easy way to extract the term and coefficient from an operator that we know has only one term and coefficient (a pattern that comes up a lot). Because currently there are some places in the code where we do this: term = list(my_term.terms.keys()[0]) coefficient = my_term[term]

It would be much better to be able to write: term, coefficient = my_term.pop_single_term() And have an error be raised if there is more than one term in my_term.

@damiansteiger how do you feel about this? We would want to keep the parallel structure between FermionOperator and QubitOperator and thus we should update the ProjectQ QubitOperator class as well if we do make these changes.

damiansteiger commented 7 years ago

I agree that it makes sense to keep FermionOperator similar to QubitOperator especially as we have already discussed once to maybe move FermionOperator to ProjectQ at a later point.

FermionOperator exposes it's public terms member which is just a dictionary and easy to use. I would suppose that most user code is mostly initializing, adding, or multiplying FermionOperators and especially QubitOperators. Library developers will need access to op.terms and it is easiest if they get just a plain python dictionary. Implementing identical magic methods as a python dictionary at the level of FermionOperator is redundant. And slightly changing the magic method behaviour will just confuse python developers who are used to the standard dictionaries.

Regarding your examples:

for term in my_operator.terms: coefficient = my_operator[term]

I don't think this is too bad but at several places we also used the following Python2 and Python3 compatible code

from future.utils import iteritems
for term, coefficient in iteritems(my_operator.terms):
    #Do something

Your second example

term = list(my_term.terms.keys()[0]) coefficient = my_term[term]

can be handled easily using the tuple assignment:

(term, coefficient), = my_operator.terms.items()

This will work again both in Python2 and Python3. In addition if terms has more than one key,value pair it will throw an exception hence it is much safer to use than the original code snippet you found and should be replaced

babbush commented 7 years ago

Your solution to use (term, coefficient), = my_operator.terms.items() is both elegant and helpful. We should definitely be doing that and I agree no method like .pop_single_term() is needed. Thanks for the good suggestions.

I do however still think that it would be nicer to have an iter method. I also feel that is very pythonic.

But I do object to your distinction between developers and users of FermiLib. Our current FermiLib code is just the beginning. Currently, this code is written for the developers, not the users. I do not think it is currently helpful to make a distinction between developers and users. This is also why I very strongly feel that develop should be the default branch on GitHub.

damiansteiger commented 7 years ago

We are exposing the my_operator.terms as a public member so developers can and should use the standard python iterators for dict. Introducing another iterator (either the same or slightly different) in the parent class FermionOperator is redundant and would only increase code dependencies which are not necessary.

As you feel strongly about the develop branch being the default branch, we have changed this.