Single responsibility principle

[thomashaener]

These are the things which need to be addressed. @babbush Could you have a look & edit where necessary?

FermionTerm:

• [x] bravyi_kitaev_transform --> transformations/
• [x] jordan_wigner_transform/sparse --> transformations/

FermionOperator:

• [x] bravyi_kitaev_transform --> transformations/
• [x] jordan_wigner_transform/sparse --> transformations/
• [x] get_interaction_operator --> transformations/

InteractionOperator:

• [x] get_fermion_operator --> transformations/
• [x] jordan_wigner_* --> transformations/
• [x] get_sparse_operator --> @babbush can we delete this or do you want to implement the direct routine in transformations as well? Right now it's just two sequential transformations...

InteractionRDM

• [ ] *expectation* --> @babbush what do you recommend here? I'd recommend having, e.g., all expectation-related functions in the same folder, as the interaction between classes seems similar to transformations.

QubitTerm

• [x] reverse_jordan_wigner --> transformations/
• [x] get_sparse_operator --> util/ or something similar @babbush ?
• [x] eigenspectrum--> delete, we can just call .eigenspectrum() on the sparse operator

QubitOperator

• [x] reverse_jordan_wigner --> transformations/
• [x] get_sparse_operator --> util/ or something similar @babbush ?
• [x] eigenspectrum--> delete, we can just call .eigenspectrum() on the sparse operator
• [ ] expectation --> same as for InteractionRDM above
• [x] get_interaction_rdm --> transformations/

[thomashaener]

@babbush I think it would be nice if this were enforced everywhere.

[babbush]

I agree. I will help work on enforcing this. By the way, I don't know who put eigenspectrum as a method on qubit operator; it wasn't me!

[thomashaener]

Another example is commutator, which is always implemented the same way... self * other - other * self so it will work for terms and operators; no need to have it in both local_operators and local_terms.

[babbush]

Hi @thomashaener . I spoke with @damiansteiger about this at length today. I don't think I fully understood the design change you were suggesting until my conversation with Damian. I understand now that you are advocating for us to change the way we are think about a lot of the library.

Let's talk about .jordan_wigner_transform(). One can call .jordan_wigner_transform() on an InteractionTensor, a FermionTerm or a FermionOperator. Each method does the expected thing but is implemented very differently. The .jordan_wigner_transform() on InteractionTensor is specific to that data structure and has exactly nothing in common with the .jordan_wigner_transform() called on FermionTerm. I understand why you might prefer us to have a separate module with a functio nrthat one can call on these classes such as jordan_wigner_transform(FermionOperator) or jordan_wigner_transform(InteractionTensor) Personally, I think that would complicate the code more than simplify it. Because now jordan_wigner_transform() needs to be a very elaborate function that has conditions on the input to switch it between two completely unrelated modes of operation. But I understand the counterargument: as currently implemented, classes tend to get pretty complicated because there are so many complex methods.

But I want to explain that this is a design decision we made early on that currently touches almost all aspects of the library. For instance, we could have had just one data structure for InteractionRDM and InteractionTensor because they both store data in essentially the same way. But in one case, the class represents an RDM and in one case, the class represents an operator. So there are very different methods that one calls on these two classes. Thus, having them as different classes is the way that we choose to organize the functions. If we enforced the single responsibility principle then it might be unclear when it is appropriate to call a certain method. For instance, if we had these as one class then somebody could call .jordan_wigner_transform() on an instance of that data structure representing an RDM. And that might sort of make sense. But the way you map RDMs to qubits and the way you map operators to qubits is very different, even though both of those things use the same underlying data structure. Our idea was that it is helpful and more intuitive for the user if the functions which are specialized to a certain class are just implemented as methods on that class. I think this will help people pick up the library faster.

But I could be wrong and this is a topic worth discussing. However, I do not think now is the appropriate time to seriously considering making this change. Because we have only two weeks left before the launch deadline and changing this opens a huge can of worms. For instance, in the example above, if we enforced the single responsibility principle then it wouldn't make much sense to have both an InteractionOperator and InteractionRDM class. So huge parts of the library would need to be reorganized. And simply put, things work well enough right now that this is not a high priority item. So I think we can pick up this conversation again as a possible change for FermiLib 0.2 but it is just not worth the enormous amount of pain it would cause to enforce with just two weeks left. And doing an incomplete job of enforcing this principle would make the general library organization inconsistent and thus unintuitive for users. Damian agreed with me that this should not be a priority before launch, do the amount of work involved. Hopefully you are okay with that?

[damiansteiger]

Just as a note: I will enforce single responsibility for stuff going into ProjectQ repo (QubitTerm and QubitOperator).

My point was that transformations like Jordan-Wigner should be separate from the data structure and hence not in, e.g., FermionOperator. But we can work on important things first.

[thomashaener]

Let's get to work :) I'll update this issue in a few minutes with individual tasks.

[damiansteiger]

@thomashaener Do you want this to happen in a new branch or master?

[thomashaener]

I don't care -- whichever is easier. This should take a few hours at most.

[babbush]

I think that get_sparse_operator is an example of a transform. It transforms between two classes anyways. I actually kind of think we should keep expectation in the RDM class. Is that really offensive? If so, let's just put it in some util folder. Should @idk3 dive into this?

[idk3]

I've fixed up everything except for the two expectation values parts - not sure if my updates to the checklist appear for you :P Was surprisingly difficult because of the amount of circular dependency (existing imports in functions + JW showing up everywhere). I'll try to refactor a bit more in the morning.

[babbush]

Great! Thanks Ian!

On Mon, Apr 17, 2017 at 10:14 PM, idk3 notifications@github.com wrote:

I've fixed up everything except for the two expectation values parts - not sure if my updates to the checklist appear for you :P Was surprisingly difficult because of the amount of circular dependency (existing imports in functions + JW showing up everywhere). I'll try to refactor a bit more in the morning.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/babbush/fermilib/issues/72#issuecomment-294682405, or mute the thread https://github.com/notifications/unsubscribe-auth/ANlTfyAghKG99AlR3ec4ELPXzP3tpV-Uks5rxEasgaJpZM4M6G7r .

[thomashaener]

Thanks Ian! It looks a lot better now :) Please let me know when you're done so I can have a look at the overloading stuff.

Another suggestion is to move all functions in transforms into different files in a new folder (subpackage) named 'transforms' which then behaves exactly the same as it does now (i.e., you can still do from transforms import ...).

In particular, I suggest moving the different transformations into files according to their functionality. I'd name all files within the subpackage 'transforms' as '_jordan_wigner.py' etc. (leading underscore). In transforms/__init__.py you then have a line from ._jordan_wigner import jordan_wigner_term_sparse, jordan_wigner_operator_sparse to populate the namespace with all functions that should be accessible from outside. I think this would be a lot nicer than having one large 700-line file ;-) We could even have separate files for the different jordan_wigner functions.

@idk3 please let me know if you'd like to do this or if I should go ahead once you're done with refactoring.

[idk3]

Hey @thomashaener,

Go ahead whenever! I'll continue with refactoring later / won't be able to start for a bit today. I agree about the transforms file ending up super long and having some pretty different components that should be separate.

[thomashaener]

Ok, then I'll do that tomorrow after our call. @babbush Do you want me to put all expectation-related functions into a util folder?

[babbush]

Yes! Sorry for the delay. Was in unavoidable meetings in Santa Barbara all day.

[thomashaener]

@idk3 I'll move all transforms into a folder now. This shouldn't change anything but the folder structure. It might take a while until I'm done with refactoring all tests though...

[idk3]

@thomashaener have you changed this? I'm looking to integrate the new FermionOperator with the transforms but it'll involve a lot of edits to them.

[thomashaener]

@idk3 I'm done with most of it and it makes more sense to finish it after FermionOperator is integrated. I pushed all changes. Just let me know when I can finish up the last few things (and do a linter run).

There is still eigenspectrum that I'd remove altogether, what do you think @babbush?

Also, why is jordan_wigner_sparse in transforms, but all other sparse JW methods are in sparse_operators.py?

[idk3]

I see - go for the final changes (linter etc) whenever, I'll try to integrate today, and you can do the final polishing after?

On Sat, Apr 22, 2017, 04:46 Thomas Haener notifications@github.com wrote:

@idk3 https://github.com/idk3 I'm done with most of it and it makes more sense to finish it after FermionOperator is integrated. Just let me know when I can finish up the last few things (and do a linter run).

There is still eigenspectrum that I'd remove altogether, what do you think @babbush https://github.com/babbush?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/babbush/fermilib/issues/72#issuecomment-296358479, or mute the thread https://github.com/notifications/unsubscribe-auth/AKRqndvJSAtRRjNPZchgZDCO2PkavB1Sks5ryb5cgaJpZM4M6G7r .

[thomashaener]

Sounds perfect. I'll do the polishing tomorrow (my time) then. Thanks!

[thomashaener]

@babbush There is still eigenspectrum that I'd remove altogether, what do you think? Where would you put jordan_wigner_sparse? It is currently in transforms, but all other sparse JW methods are in sparse_operators.py, so either we move all of them to transforms or put jordan_wigner_sparse into sparse_operators as well...

[babbush]

Hi Thomas,

Whatever you think makes more sense is fine. You can get rid of the eigenspectrum thing.

On Sun, Apr 23, 2017 at 2:26 AM, Thomas Haener notifications@github.com wrote:

@babbush https://github.com/babbush There is still eigenspectrum that I'd remove altogether, what do you think? Where would you put jordan_wigner_sparse? It is currently in transforms, but all other sparse JW methods are in sparse_operators.py, so either we move all of them to transforms or put jordan_wigner_sparse into sparse_operators as well...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/babbush/fermilib/issues/72#issuecomment-296430586, or mute the thread https://github.com/notifications/unsubscribe-auth/ANlTf29hQg0zk4ZLTeuRrmFrAjGIXA2aks5ryxlVgaJpZM4M6G7r .

[thomashaener]

Ok great. I think that all sparse operator related code should go into util/. What do you think @idk3 ? That would be the last change I'd say... :)