search

babbush / HistoricalFermiLib

This is repo where we developed FermiLib, which then became OpenFermion
Apache License 2.0
1 stars 0 forks source link

Enforce single-responsibility in sparse operators #76

Closed babbush closed 7 years ago

babbush commented 7 years ago

I am not happy with enforcing this principle in some places but not others. If we are going to do it, we must do it everywhere. @thomashaener since this is your thing, I'm assigning you.

thomashaener commented 7 years ago

I don't really see the responsibility of sparse operator to be honest. It is just a wrapper for a sparse matrix which forwards all methods to scipy... Enforcing single responsibility would mean getting rid of this class. I would still leave the conversion methods in transforms, allowing to generate a sparse matrix from an operator.

Please let me know if I'm missing something / if you want to keep this class. I won't be able to do it before tomorrow anyway.

babbush commented 7 years ago

I see your point. Perhaps we should make this entire module into a utils which just has all sorts of functions that interact nicely with the sparse matrix classes. We can keep the conversions into the sparse matrix class. So maybe we can get rid of it to some extent. What do you think?

thomashaener commented 7 years ago

Yes, but then we might want to move all conversion functions interacting with the sparse matrix class to that util folder as well. Or we can just move sparse operator to utils/, either is fine with me.

babbush commented 7 years ago

If you still have time implement the change in a nice way then I think it would be great to move just the sparse operator stuff into utils but only if you were going to implement the change all the way and get rid of the sparse operator class but retain all the functions we wrote that acts on scipy.sparse matrices. It is really important to be able to get the eigenspectrum of a FermionOperator, for instance. Short of this, it's probably best to just leave things how they are. Up to you.