Add lattice and Fermi-Hubbard model

[k-tamuraphys]

Summary

Implement lattice and Fermi-Hubbard model

Details and comments

Gist URL of the jupyter notebook for the lattice module:
https://gist.github.com/k-tamuraphys/6da54c468fffca3b4d18d9837de81aaf

To Do

• improve adjacency_matrix
• add the defaults position of the draw method
• add unit tests
• add release note

Added by @ikkoham

[CLAassistant]

All committers have signed the CLA.

[mtreinish]

Two quick comments here, first, you'll need to add retworkx to https://github.com/Qiskit/qiskit-nature/blob/main/requirements.txt since it's being explicitly imported by nature now. The second is the lint failures with retworkx are because pylint can't do it's own parsing and ast analysis of a compiled extension, you need to add retworkx to the extension pkg list in the pylintrc: https://github.com/Qiskit/qiskit-nature/blob/main/.pylintrc#L37 so pylint knows to not try to inspect the members

[k-tamuraphys]

Two quick comments here, first, you'll need to add retworkx to https://github.com/Qiskit/qiskit-nature/blob/main/requirements.txt since it's being explicitly imported by nature now. The second is the lint failures with retworkx are because pylint can't do it's own parsing and ast analysis of a compiled extension, you need to add retworkx to the extension pkg list in the pylintrc: https://github.com/Qiskit/qiskit-nature/blob/main/.pylintrc#L37 so pylint knows to not try to inspect the members

Thank you for the comment. I added retworkx to requirements.txt and .pylintrc.

[mrossinek]

Thank you @k-tamuraphys for the hard work on this! Some comments from my side (rather nitpicky at times...)

I did not look at the unittests in detail but will do so on the second iteration of my review 👍

[mrossinek]

It's getting increasingly difficult to find remaining unresolved conversations. I included two screenshots below. Other than that, this lgtm now 👍

[ikkoham]

TODO:

• Add lattice and Fermi-Hubbard model #365 (comment)

[pbark]

I think that now this LGTM except the minor naming change that we discussed above.

[ikkoham]

I changed the file path according to @pbark's comment. 138de92

[woodsp-ibm]

Since self.graph already does a copy() this seems to be doing a copy of the copy. What is this expected to be used for - I am asking since it seems to be in this base class but not overridden (that I saw) in any derived classes. Hence if I call copy() on a derived instance, say a HyperCubicLattice, I will not get a copy of that rather I am going to get a base class Lattice instance using its graph. Since user can make a Lattice from the graph property easily themselves too, with the above observations I guess I am questioning whether having this method is useful

[woodsp-ibm]

In thinking/reviewing a little more I wondered about the sub-classes since it seemed they just created a graph for the Lattice constructor and only a couple had a little more. I was wondering if all those classes, all those types, were actually needed/useful - rather than say just have factory functions that build out a Lattice in square, triangular form etc. In a couple of the cases it seems that the parameters passed to the constructor are used in the draw without boundary. Otherwise these sub-classes do not add any new/specialized functions at for the derived types. Like for instance being able to add one more row to the lattice. Now I see there are public variables for rows, cols etc in sub-classes. While I can see reading these as properties can be useful when there are these subtypes, they are public instance vars, not properties like is done in Lattice, and thus they can be set too. Which will either have no effect (square lattice) or later mess up the draw without boundary. Should all these be private instance vars and have properties just to read them from a public API perspective. Maybe these sub-types can be expanded upon in the future so it seems more useful to have them as specific types - at present, to me, aside from draw without boundary, that maybe cannot be done in Lattice itself from the graph, their existence just adds a bunch of types/classes with no other additional specilalization/value beyond that.

[ikkoham]

I think this is correct about copy. (or copy.deepcopy might be safer.)
8beeb63#diff-f5c282fdc2a29ac3872af2c25326ad98d8277544a967e89fb6dec888b872c5c9R151
I choose to use deepcopy here.
36367eb

For public API, I'm sure there were unnecessary setters, so I deleted it and made them getters only.
8af7388

Currently, there is no special method, but the meaning is clear from the class name, so I believe it is easier to use than, for example, creating from a class method.

[woodsp-ibm]

For public API, I'm sure there were unnecessary setters, so I deleted it and made them getters only.

For the hyper_cubic_lattice is still seems to have a dim and size public instance vars.

Currently, there is no special method, but the meaning is clear from the class name, so I believe it is easier to use than, for example, creating from a class method.

Ok, I was more thinking of public methods say square_lattice(.....) -> Lattice. But perhaps, even though the various types today do not have any specialization beyond the one method in a couple of the classes, in the future that will happen and then the structure is more suitable for this than say just having methods.

[pbark]

@woodsp-ibm thanks for the comment. In general I agree with @ikkoham that the current implementation is more user friendly where the class names correspond to the expected lattice.

[ikkoham]

I forgot dim and size. I'll add them soon.

As for type hint, they are fine for returning sub classes (Liskov). If you need in the future, we can override stronger type hints in the sub class side.

[woodsp-ibm]

For copy() that's fine - what you have now is a bit different than before in that now its making a copy of the instance, and hence whatever the subclass is, not a new Lattice (possibly parent of instance) with the same graph.

[ikkoham]

Oh my... Python 3.6 hasn't been removed yet.