Skip to content
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

Cleanup a few methods that aggregate FermionOperators #40

Merged
merged 5 commits into from
May 23, 2017

Conversation

Strilanc
Copy link
Collaborator

@Strilanc Strilanc commented May 20, 2017

Pairs like ((p, 1), (q, 0)) seem to be very common throughout the code. This PR turns that pattern into a method. I also added an anti_hermitian argument, since that was used in a few places.

I assume move is a terrible name. Please suggest something better if so. annihilate_create is probably appropriate, but a bit verbose.

These changes, plus itertools.product, simplified uccsd_singlet_operator quite a lot.

@Strilanc
Copy link
Collaborator Author

Strilanc commented May 20, 2017

@babbush Are FermionOperators sensitive to order? isclose seems to return false when comparing 1 2^ and 2^ 1.

@idk3
Copy link
Collaborator

idk3 commented May 20, 2017

They are sensitive to order - the reason is that swapping two fermions applies -1 to the state. In this case, 1 2^ = -2^ 1, but we don't check for that in equality (the reordering is potentially very costly). We have a method normal_ordered() which swaps the creation and annihilation operators into a canonical ordering that can be used - if you normal order 1 2^ and -2^ 1 you should find those are equal. There's a bit more including some examples on Wikipedia.

@Strilanc
Copy link
Collaborator Author

Ah, this explains some of the test failures then. Sometimes you do ++--, but other times you do +-+-. I was assuming they commuted.

@Strilanc
Copy link
Collaborator Author

Strilanc commented May 21, 2017

I've cut the double-move, since it didn't fit cleanly.

@Strilanc Strilanc changed the title [WIP] Add annihilate+create factory methods to FermionOperator Add annihilate+create factory methods to FermionOperator May 21, 2017
@damiansteiger damiansteiger self-requested a review May 21, 2017 04:10
Copy link
Contributor

@babbush babbush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems like you are creating a term like a^\dagger_p a_q. I would just call that "one_body_term". This is another method we had back in the day but intentionally deleted. But we can put it back if you want. However, I do not understand why you would make this as a static method as opposed to just a function that returns a term. Also, let's not called the boolean "anti_hermitian" but instead just have a parameter called "add_conjugate". The idea is that we would add a_q^\dagger _a_p to a_p^\dagger a_q if this option is selected but only if p != q. The use of itertools in the ucc code is clearly a good idea.

@babbush
Copy link
Contributor

babbush commented May 22, 2017

Actually I kind of see why making this a static method makes sense. I am okay with that.

Copy link
Contributor

@damiansteiger damiansteiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of changes to FermionOperator unless they are totally necessary as this core data structure will once move to ProjectQ in order to increase the speed with a C++ implementation...

I am currently not yet convinced that we need the annihiliate_create function because:

  1. I can achieve the same things very fast using the standard initializer of FermionOperator
  2. If I read in the code:
FermionOperator.annihilate_create(3, 1, 8.17)

I don't really know what is going on because the function name is not descriptive enough (I don't think one could choose a better one to solve the problem). So I would have to look it up in the doc string which says:

FermionOperator: A paired annihilation-creation
operator.

which still doesn't tell me what is going on except if I read the function code

If you would, however, use the current normal FermionOperator initializer using strings, I think it would be clearer what the above line does

8.17 * (FermionOperator('3^ 1') + FermionOperator('1^ 3'))

and not much more to write.

It would be a different topic if you think the string initializer is not nicely human readable, so then we would have to discuss that in an issue to find a better solution

@Strilanc
Copy link
Collaborator Author

I agree that this method is not great yet. I will probably cut this PR down to just the reductions in size.

That being said...

"It looks like the normal notation we use" is kind of a bad sign of a reason to give. Programmers often go through a phase when learning a new language where they "write C in python", writing code with conventions that made sense in the old language but don't quite fit the new language. They kinda end up trying to use a drill like it was a screwdriver.

When you give arguments like "it looks like X!" it seems like you're making the same mistake, but with mathematical notation. Don't try to make the code look like a notation optimized for use on paper, do a proper translation of the underlying meaning into the new context that fits within the whole of the target language. Don't bend your tools, bend your use.

For example, you have this text format like "1^ 3 2^ 4", but all the code making these things has the modes as variables, not constants. It would be more work to make the concise string than to make the tuples, and the parsing adds unnecessary overhead. It seems strictly inferior to, e.g., using the non-negative int p for raising mode p and its bitwise complement ~p for lowering mode p.

I also feel a bit strongly that the current solution is very bleh for reading or writing. Maybe you get used to the fact that 0 is up and 1 is down and no other ints mean anything (so shouldn't it be True/False instead of 0/1?), but I haven't yet. And the way code just all shares implicit expectations on how these things are structured means you have to keep repeating the expected format in all the method docstrings.

This kind of problem, "how do I represent my data?", is really what classes are designed for. So we should strongly prefer to use classes for it. As a first step, we should transition the (mode, raising) tuple to a nametuple.

@babbush
Copy link
Contributor

babbush commented May 22, 2017

Hi Craig,

I disagree with the following statement:

When you give arguments like "it looks like X!" it seems like you're making the same mistake, but with mathematical notation. Don't try to make the code look like a notation optimized for use on paper, do a proper translation of the underlying meaning into the new context that fits within the whole of the target language. Don't bend your tools, bend your use.

FermiLib is a tool for physicists and part of the philosophy of this library is that it should be intuitive and easy to use to those who are familiar with the equations that we are realizing in code. You are not used to 0 being lowering and 1 being raising but you are also not familiar with manipulating fermionic operators and everybody who uses this code most likely is. We are not going to change the ladder operators to a new class right now. There are many, many, many things that need to be done in FermiLib. This is not a priority and would be a lot of work. We like how the data structure works now.

@babbush babbush closed this May 22, 2017
@babbush babbush reopened this May 22, 2017
@Strilanc
Copy link
Collaborator Author

Strilanc commented May 22, 2017

@babbush Prefix a paragraph with > to quote it.

quote

I agree that there is value in choosing notation that physicists will recognize. Sometimes it is worth doing something awkward to make it familiar, although then people will tend to replace your tool over time... but I guess that would kind of count as success in this context.

I don't think that (p, 1) is an example of a "familiar" notation. I would never guess that the 1 was a raised dagger.

@Strilanc
Copy link
Collaborator Author

Just to recap: current plans are that I'll cut the method and keep the other cleanups. Adding [WIP].

@Strilanc Strilanc changed the title Add annihilate+create factory methods to FermionOperator [WIP] Add annihilate+create factory methods to FermionOperator May 22, 2017
@Strilanc Strilanc changed the title [WIP] Add annihilate+create factory methods to FermionOperator [WIP] Cleanup a few methods that aggregate FermionOperators May 22, 2017
@Strilanc Strilanc changed the title [WIP] Cleanup a few methods that aggregate FermionOperators Cleanup a few methods that aggregate FermionOperators May 22, 2017
@Strilanc
Copy link
Collaborator Author

Ready for review again.

@Strilanc
Copy link
Collaborator Author

@damiansteiger You need to dismiss your review if you're okay with this being merged now that it has a much smaller scope.

@damiansteiger
Copy link
Contributor

Don't worry, I am aware of all the pull request for which I assign myself ;-)

@damiansteiger damiansteiger self-requested a review May 23, 2017 03:01
@babbush babbush merged commit 6de3144 into ProjectQ-Framework:develop May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants