Update API to allow QubitMapper where QubitConverter is used. #999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Anthony-Gandon
requested review from
stefan-woerner,
manoelmarques,
woodsp-ibm and
mrossinek
as code owners
mrossinek
reviewed
Dec 8, 2022
qiskit_nature/second_q/algorithms/excited_states_solvers/excited_states_eigensolver.py
Outdated
Show resolved
Hide resolved
| @@ -34,7 +34,7 @@ class ExcitedStatesEigensolver(ExcitedStatesSolver): | |||
|
|
|||
| def __init__( | |||
| self, | |||
| qubit_converter: QubitConverter, | |||
| qubit_converter: Union[QubitConverter, QubitMapper], | |||
There was a problem hiding this comment.
In the future I would like to rename this argument to qubit_mapper. This will not be an easy process so it will be done in a separate PR once all the API is updated to support QubitMapper and works. I'm mentioning it now just as an FYI.
This will not be an easy undertaking but I will start discussing some plans on how to do this properly offline.
qiskit_nature/second_q/algorithms/excited_states_solvers/excited_states_eigensolver.py
Outdated
Show resolved
Hide resolved
| @@ -58,7 +58,7 @@ def get_qubit_operators( | |||
| ) -> Tuple[PauliSumOp, Optional[dict[str, PauliSumOp]]]: | |||
| """Construct qubit operators by getting the second quantized operators from the problem | |||
| (potentially running a driver in doing so [can be computationally expensive]) | |||
| and using a QubitConverter to map and reduce the operators to qubit operators. | |||
| and using a QubitConverter or QubitMapper to map and reduce the operators to qubit operators. | |||
There was a problem hiding this comment.
We will need to do a thorough pass of the docs. Just mentioning this so its on our radar (I know that this is still a draft PR)
qiskit_nature/second_q/algorithms/excited_states_solvers/qeom_vibrational_ops_builder.py
Outdated
Show resolved
Hide resolved
releasenotes/notes/update-api-for-qubitmapper-qubitconverter-d237a7b596d50207.yaml
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 3856020053
💛 - Coveralls |
mrossinek
reviewed
Dec 21, 2022
releasenotes/notes/update-api-for-qubitmapper-qubitconverter-d237a7b596d50207.yaml
Outdated
Show resolved
Hide resolved
|
New unit tests were added following the API changes, however I still need to review properly the docstrings. |
Anthony-Gandon
changed the title
mrossinek
reviewed
Jan 5, 2023
qiskit_nature/second_q/algorithms/ground_state_solvers/ground_state_eigensolver.py
Show resolved
Hide resolved
| @@ -63,20 +63,21 @@ def __init__( | |||
| ) | |||
|
|
|||
| @property | |||
| def qubit_converter(self) -> QubitConverter: | |||
| def qubit_converter(self) -> QubitConverter | QubitMapper | None: | |||
There was a problem hiding this comment.
This currently still holds QubitConverter(DirectMapper()) as the internally-generated default
value. Can we already change this or do we need to keep this as is?
If the latter, shall we log a warning that this default is going to change?
Also pinging @woodsp-ibm
There was a problem hiding this comment.
Having the default value set to DirectMapper() won't change any of the logic. However, because it is a public attribute, it might be possible that one defines a VSCF object with the default value and then affects symmetries to the qubit converter. In that case, the QubitMapper will fail.
test/second_q/algorithms/excited_state_solvers/test_excited_states_solvers.py
Outdated
Show resolved
Hide resolved
test/second_q/algorithms/excited_state_solvers/test_qeom_electronic_ops.py
Show resolved
Hide resolved
test/second_q/algorithms/excited_state_solvers/test_qeom_vibrational_ops.py
Show resolved
Hide resolved
…added new tests to cover more cases
woodsp-ibm
approved these changes
Jan 11, 2023
Anthony-Gandon
added a commit
to Anthony-Gandon/qiskit-nature
that referenced
this pull request
May 25, 2023
…-community#999) * Failing tests but stable QubitConverter and QubitMapper can now be interverted * Modifications of the mappers map() * Update API for QubitMapper * Adds release note * Small corrections after first feedback. * Fix indentation: * Change for map_all to map and map to map_single. * Updated release note * Updated release note 2 * Small updates before pushing * small fix spell * First review of docstrings * Other docstrings * Make black * Fix mypy * Fix comments * fix * Rolled back the type expansion of the map and convert_match methods, added new tests to cover more cases * Implement the unwrapper in the _ListOrDict class * Fix spelling * Fix spell 2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #972
This PR updates the API to allow
QubitMapperobject whereQubitConverterwere previously required. THis is possible because theQubitConverteris a wrapper around theQubitMapperwith additional methods for qubit reduction (two qubit reduction and tapering). The utility methodmap()was modified in the QubitMapper to act on lists, as theconvert_match()does for the QubitConverter.The following gist goes through a the main classes where this functionality is accessible: gist
This PR is going to be affected by #971, but I wanted to initiate the reflexion and feedback process.