search

quantumlib / OpenFermion

The electronic structure package for quantum computers.
Apache License 2.0
1.51k stars 372 forks source link

Jellium model generation #316

Closed kevinsung closed 6 years ago

kevinsung commented 6 years ago

I asked @idk3 about generating Jellium models and he pointed me to the function dual_basis_jellium_hamiltonian in utils/_low_depth_trotter_error.py. Doesn't this belong in hamiltonians/_jellium.py?

babbush commented 6 years ago

Absolutely. I have no idea how that got there. It must have been moved by @jarrodmcc recent pull request. Jarrod was there some reason we should be aware of, or some reason we might want to not use that function or something to generate jellium models?

kevinsung commented 6 years ago

Actually, @idk3 told me he put it there. @idk3 would you mind moving it to the right place? I think it might need a new name and a better docstring too.

babbush commented 6 years ago

so I don't know what kind of tricks @idk3 is up to but I really don't think that should be there. @kevinsung there is also code in the right place, in hamiltonians/_jellium.py with the name dual_basis_jellium_model and that is almost certainly the function I would trust and recommend you to use.

idk3 commented 6 years ago

Indeed - the first time I tried to add this, @babbush told me to "get rid of this function, it is redundant with the normal jellium_model code." The problem is that it's a 20-line function that you need every time you want the dual basis jellium Hamiltonian. IMO it reflects a more general restructuring that we might want to think about with jellium_model - the problem is the dependence of jellium_model on 1. Grid and 2. wigner_seitz_length_scale. The user has to work with all three to get the Hamiltonian (including figuring out where to import them from, every time) rather than just a single function.

dual_basis_jellium_hamiltonian was my way of sneaking this functionality in to save a lot of headaches in the Trotter error code - it allows the user to do a direct definition without having to compute the appropriate length scale and grid, number of particles, etc, etc. Were it not for Grid's use in Fourier transforming, my ideal would be to not expose users to it and have dual_basis_jellium_hamiltonian-style functions be the user-facing way to call these Hamiltonians.

babbush commented 6 years ago

@idk3 I disagree with the premise of this function, but whether it should exist is fair to debate. However, having this function hidden in your Trotter error code is unacceptable. You know it doesn't belong there! Please move it immediately.

idk3 commented 6 years ago

As long as it stays in! ;) https://github.com/quantumlib/OpenFermion/pull/320 addresses this.