-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Slater determinant and fermionic Gaussian state initial states #483
Add Slater determinant and fermionic Gaussian state initial states #483
Conversation
The pylint rules are common throughout qiskit and hence we should try and find variable names etc that fit within the current rules rather than changing the rules. |
Very well. I reverted the pylint rules, and instead disabled the checks for the violating lines by adding comments. |
Ok, it would be more acceptable though to choose names etc within the rules rather than disabling them to skirt the rules. Having to disable an exceptional case where pylint is having trouble but things are as needed does happen, but disabling the rule inline should not be the norm here. |
2fbe7a2
to
eec6b8a
Compare
07e4abf
to
0359113
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comments. I assume you will refactor #482 according to our offline discussion so I am holding off with reviewing that PR
qiskit_nature/circuit/library/initial_states/fermionic_gaussian_states.py
Outdated
Show resolved
Hide resolved
qiskit_nature/circuit/library/initial_states/fermionic_gaussian_states.py
Outdated
Show resolved
Hide resolved
qiskit_nature/circuit/library/initial_states/fermionic_gaussian_states.py
Outdated
Show resolved
Hide resolved
ee2876e
to
085fd70
Compare
085fd70
to
6d766fc
Compare
@woodsp-ibm I've addressed your comments. |
Co-authored-by: Steve Wood <[email protected]>
test failures seem unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this final nitpicky comment, I think this is good to go! Thanks a lot, @kevinsung!
docs/tutorials/11_quadratic_hamiltonian_and_slater_determinants.ipynb
Outdated
Show resolved
Hide resolved
@mrossinek I addressed one of your comments and wasn't sure how to address the other. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this makes things clearer
docs/tutorials/11_quadratic_hamiltonian_and_slater_determinants.ipynb
Outdated
Show resolved
Hide resolved
docs/tutorials/11_quadratic_hamiltonian_and_slater_determinants.ipynb
Outdated
Show resolved
Hide resolved
@mrossinek I disagree with your suggestions because they make it seem like the creation and annihilation operators are independent of each other. I think the set notation {a_j} should only include one or the other, not both. |
Specifically, the notation {a_j, a_j^\dagger} makes it appear that a_j^\dagger is an independent symbol rather than simply the hermitian conjugate of a_j. |
In that case, please make it consistent whether you define the creation or annihilation set because right now you define |
@mrossinek Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this lgtm now. @woodsp-ibm please also take a look 👍
@mrossinek Since it looks good to you, could you please approve it? You just left a comment without actually approving it. |
…iskit-community#483) * add QuadraticHamiltonian class * preparation of Slater determinants and fermionic Gaussian states * validate input * expose _fermionic_op as to_fermionic_op * use tuple labels and merge loops * update docs * add num_modes argument * document new errors * add num_modes property * update doc * spelling * mypy * from __future__ import annotations * add release note * trigger CI * fix spelling * update terra * split so each class has own file * fix comments and error messages, don't name registers * revert library init description * run copyright check * add release note * spelling * spelling and docs * make private functions hidden again * mypy * improve docs and error messages * split test file * add input validation * mypy * lint * add slater determinants test file * create utils module * test unsupported mappers fail gracefully * fix imports * raise NotImplementedError for unsupported mapper * improve docs * rewrite documentation * improve docs and error messages * factor out validation function * improve slater determinant docs * add tutorial notebook * typo * fix warning in notebook * edit notebook * comma * spelling and format * add note to QuadraticHamiltonian class * Update docs/tutorials/11_quadratic_hamiltonian_and_slater_determinants.ipynb Co-authored-by: Max Rossmannek <[email protected]> * add hyperlinks * update print output in notebook * Update releasenotes/notes/slater-53415163fff84313.yaml Co-authored-by: Steve Wood <[email protected]> * format * edit notebook kernel to satisfy CI * edit notebook * edit notebook to say creation instead of annihilation Co-authored-by: Panagiotis Barkoutsos <[email protected]> Co-authored-by: Manoel Marques <[email protected]> Co-authored-by: Max Rossmannek <[email protected]> Co-authored-by: Steve Wood <[email protected]> Co-authored-by: Max Rossmannek <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
Fixes #459 . Comes after #462 and #482.
Details and comments
I made the following changes to the pylint rules:
_prepare_fermionic_gaussian_state_jordan_wigner
p
to the list of allowed variables since it is a common name for an index or number.Let me know if this is not acceptable.