-
Notifications
You must be signed in to change notification settings - Fork 207
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
Qeom refactoring and upgrades #971
Qeom refactoring and upgrades #971
Conversation
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 think this PR is overall already in great shape. Thanks a lot Anthony for all the work here!
These are quite a few comments and I will need to do another pass of the actual QEOM code in a little bit more detail with some more time on my hands. But I figured I should post my review as it is now to give you time to work on these comments.
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom_electronic_ops_builder.py
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/ground_state_solvers/ground_state_solver.py
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
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.
A few more comments from my side 👍
This is shaping up very nicely! I am skipping the changes done to the QubitConverter
on purpose, since it will be refactored again as part of #967 at which point we can revert these unreleased changes and update the QEOM code accordingly. I would still like to get this long-standing PR out before the QubitConverter
refactor hits because it allows us to do these two things largely in parallel 👍
(the above is just a comment for a potential other reviewer coming across this)
qiskit_nature/second_q/algorithms/excited_states_solvers/excited_states_solver_eval_rules.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/ground_state_solvers/ground_state_eigensolver.py
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qeom_result.m_matrix = h_mat[: len(h_mat) // 2, : len(h_mat) // 2] | ||
qeom_result.v_matrix = s_mat[: len(h_mat) // 2, : len(h_mat) // 2] | ||
qeom_result.q_matrix = h_mat[len(h_mat) // 2 :, : len(h_mat) // 2] | ||
qeom_result.w_matrix = s_mat[len(h_mat) // 2 :, : len(h_mat) // 2] | ||
qeom_result.m_matrix_std = h_mat_std[0, 0] | ||
qeom_result.v_matrix_std = s_mat_std[0, 0] | ||
qeom_result.q_matrix_std = h_mat_std[0, 1] | ||
qeom_result.w_matrix_std = s_mat_std[0, 1] |
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.
This is good, thank you 👍
@manoelmarques what would the deprecation strategy look like here, if I want to make these read-only properties of the result object. Doing this would allow us to put this computation logic into the property getters and ensure that the matrices stored in the result are always consistent (plus it simplifies the code here a bit).
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.
If you put the calculating logic in the init method, then you just need the deprecation msg in the setters. This way the user still can use the deprecated setter which would override the calculation done in the init.
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.
Thanks, that's a good idea 👍 @Anthony-Gandon could you do that, please?
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.
As discussed with @mrossinek , I could not put the calculating logic in the init method. I changed the getters instead.
test/second_q/algorithms/excited_state_solvers/test_excited_states_solvers.py
Outdated
Show resolved
Hide resolved
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.
This is looking very good, thanks a lot, Anthony!
I have a last round of comments, mostly on some docstring details 👍
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom.py
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
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.
Okay some final (minor) observations but other than that this looks good to me 👍
We will just need to keep in mind that, once we progress on #972 and #999, we should revert the public API changes (as much as possible) done to the QubitConverter
in this PR.
We can see about the details of that as part of working on the epic mentioned above.
qiskit_nature/second_q/algorithms/ground_state_solvers/ground_state_eigensolver.py
Outdated
Show resolved
Hide resolved
releasenotes/notes/refactors-and-upgrades-qeom-bd14d85c94e912d0.yaml
Outdated
Show resolved
Hide resolved
I'm not sure why we did not have a mypy error previously when
(because self._gscc is GroundStateSolver and does not have a _solver attribute, only GroundStateEigensolver has). Can we add a #type: ignore because anyway this part will be subject to changes in the near future. |
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 am tentatively approving this PR because I think it looks good as it is.
I am not quite sure what you meant in your last comment though, so I will hold off with merging until that is cleared up either here or offline.
EDIT: the questionable type ignore statement is already included in this PR and fine by me 👍
Thanks a lot for all your work, Anthony!
* QEOM refactoring and changes. All grouped under one commit to fix the authoring of the commits v2 * QEOM refactoring and changes. Add new files * Modify type of argument op_aux_op_dict to pass test * Make black and type check * Make black and change name * Change file name * small modifs * Fix faulty tests * Fix mypy and test * New unittest and small fixes * Fix Max comments, change tests, update release not, change groundstateeigensolver behavior * Add parameter for eigenvalue treshold, make black * Response to comments and small fixes: ground_state_eigensolver, qeom, qubit_converter * Fix lint and style * Fix comments and change docstring * changes to get_qubit_operator * changes to get_qubit_operator * Excitations in QEOM becomes public attribute * fix doctring qeom * Update excitation docstring to match UCC docstring
Closes #769
Closes #415
Closes #403
Closes #52
Summary
Proposition for a modification to QEOM internals to :
A new functionality is also added to compute excited state properties and transition amplitudes of arbitrary observables.
The typical interface to use it would be the following:
Details and comments
The following guides details some of the changes that are proposed
QEOM_refactoring-3.pdf