qiskit-community / ffsim

Faster simulations of fermionic quantum circuits.
https://qiskit-community.github.io/ffsim/
Apache License 2.0
22 stars 5 forks source link

Add one-dimensional Fermi-Hubbard model #106

Closed bartandrews closed 5 months ago

bartandrews commented 5 months ago

Fixes #102

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

bartandrews commented 5 months ago

I have fixed the lint and format errors (as per the readme instructions), and the tox runs successfully on my computer now

bartandrews commented 5 months ago

Thank you for the code review! I have implemented your suggestions and have added some tests, which benchmark the ground-state energies for an L=4 chain against known results (from pyblock3, TeNPy, etc.). The tox runs successfully on my computer

bartandrews commented 5 months ago

Thanks again for the suggested improvements -- I have implemented these changes now

bartandrews commented 5 months ago

Thanks for catching this, I have implemented these changes now. Also, I want to confirm that in the docstring, when I write :math: `\mu`, is it okay to use to use a single slash here? Sometimes the backslash needs to be escaped/doubled for proper rendering

kevinsung commented 5 months ago

Thanks for catching this, I have implemented these changes now. Also, I want to confirm that in the docstring, when I write :math: \mu, is it okay to use to use a single slash here? Sometimes the backslash needs to be escaped/doubled for proper rendering

I think the single slash is fine because it's in a raw string (the string is prefixed by r). I've just added instructions to the README for building and viewing documentation locally. Please check that the docs you wrote render correctly.

bartandrews commented 5 months ago

Thank you for adding a note on how to compile the docs locally. The mu was indeed rendered correctly, but viewing the docs allowed me to fix a different issue, which was useful. I have renamed to fermi_hubbard_1d in anticipation of a fermi_hubbard_2d in fermi_hubbard.py and corresponding functions test_fermi_hubbard_2d... in fermi_hubbard_test.py. I can implement that in a separate PR if needed

bartandrews commented 5 months ago

Thank you for catching these inconsistencies, I have implemented the changes now :rocket: The next pull request should be smoother once I have learnt your code conventions :+1: (I have also changed the test location to be in the python subdirectory.)

bartandrews commented 5 months ago

As requested, in the tests I have now "compared the dictionary of the operator with one constructed explicitly without any for loop logic", for open and periodic boundary conditions. In the process, I also noticed that some existing tests in ffsim use assert A == B rather than np.testing.assert_equal(A, B), which seems to go against your convention. Should these also be fixed?

kevinsung commented 5 months ago

assert A == B rather than np.testing.assert_equal(A, B)

The reason to use np.testing.assert_allclose is that the error message displays the values of the arrays being compared. When A and B aren't arrays then the error messages from pytest are good enough. In any case, unrelated improvements should be done in a separate PR.