Cleanup plane wave fourier transform #34
Conversation
|
Don't merge this yet (that's why [Blocked] is in the title). It needs to be rebased to expunge the non-CLA commit. |
- Use Grid parameter instead of grid field parameters - Extract common helper to handle both the forward and inverse cases
# Conflicts: # src/fermilib/ops/_fermion_operator.py # src/fermilib/ops/_fermion_operator_test.py
… u/cg/minor-9 # Conflicts: # src/fermilib/utils/_plane_wave_hamiltonian.py
… u/cg/minor-9 # Conflicts: # src/fermilib/utils/_plane_wave_hamiltonian.py
Strilanc
changed the title
|
No longer blocked. |
| transformed_term = new_basis | ||
| else: | ||
| transformed_term *= new_basis | ||
| if transformed_term is None: |
There was a problem hiding this comment.
@babbush Please double-check that removing this conditional-continue doesn't change the logic.
There was a problem hiding this comment.
@Spaceenter please advise.
| hamiltonian_t = transformed_term | ||
| else: | ||
| hamiltonian_t += transformed_term | ||
| hamiltonian_t += transformed_term |
There was a problem hiding this comment.
If transformed_term equals to FermionOperator.identity(), we should not add to hamiltonian_t?
It's equivalent to the case that the code doesn't enter the for loop in line 221, but maybe that's not possible since we don't expect empty term in hamiltonian.terms?
|
If there is a difference we should check for it with a test. Can you make a
test that distinguishes between the two cases? Ideally one that is
pragmatic enough that it's obvious which behavior we want.
…On Thu, May 25, 2017, 2:33 PM Wei Sun ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/fermilib/utils/_plane_wave_hamiltonian.py
<#34 (comment)>
:
>
# Coefficient.
transformed_term *= hamiltonian.terms[term]
- if hamiltonian_t is None:
- hamiltonian_t = transformed_term
- else:
- hamiltonian_t += transformed_term
+ hamiltonian_t += transformed_term
If transformed_term equals to FermionOperator.identity(), we should not
add to hamiltonian_t?
It's equivalent to the case that the code doesn't enter the for loop in
line 221, but maybe that's not possible since we don't expect empty term in
hamiltonian.terms?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#34 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAE4RU5J8CnI209ZkEV8rx15bNJYItBhks5r9fOwgaJpZM4NhMh2>
.
|
|
Actually, I think what you implemented should be OK, as the hamiltonian is always from the function plane_wave_hamiltonian(), so it should not contain "empty" term. |
|
Great work guys! |
Comes after #32