Skip to content
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

Inverse state mapper #154

Merged
merged 10 commits into from
Aug 9, 2023
Merged

Inverse state mapper #154

merged 10 commits into from
Aug 9, 2023

Conversation

kwkbtr
Copy link
Contributor

@kwkbtr kwkbtr commented Jul 4, 2023

No description provided.

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

@github-actions github-actions bot temporarily deployed to pull request July 4, 2023 10:18 Inactive
@kwkbtr kwkbtr mentioned this pull request Jul 6, 2023
@kwkbtr kwkbtr force-pushed the inv-state-mapper branch from de7bdfb to 6923682 Compare July 13, 2023 06:25
@github-actions github-actions bot temporarily deployed to pull request July 13, 2023 06:29 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 27, 2023 09:56 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 27, 2023 11:52 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 27, 2023 20:30 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 27, 2023 20:35 Inactive
@toru4838 toru4838 marked this pull request as ready for review July 27, 2023 21:02
@toru4838 toru4838 marked this pull request as draft July 31, 2023 11:39
@github-actions github-actions bot temporarily deployed to pull request August 1, 2023 05:27 Inactive
@toru4838 toru4838 marked this pull request as ready for review August 1, 2023 05:38
Copy link
Contributor Author

@kwkbtr kwkbtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving post-selection filter functions from quri_parts.chem to quri_parts.openfermion is a breaking change, so let's note it explicitly in pull request description.

Some mappings require this argument (e.g. symmetry-conserving
Bravyi-Kitaev transformation) while the others ignore it.
n_up_spins:
The number of spin orbitals.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring needs to be updated.

@kwkbtr kwkbtr requested a review from dchung0741 August 4, 2023 10:34
@dchung0741
Copy link
Contributor

I haven't read the post selection functions yet, but I have one question about the updated interface for incorporating the inverse mappers.
It looks like the inverse mapper could take in the n_up_spins argument.
For the SCBK Hamiltonian mapping, it also needs to take in n_up_spins in order to compute the correct parity factor for the Hamiltonians.
So my question is: Should we add the n_up_spins argument to the get_state_mapper, get_of_operator_mapper methods as well?

@kwkbtr
Copy link
Contributor Author

kwkbtr commented Aug 7, 2023

Should we add the n_up_spins argument to the get_state_mapper, get_of_operator_mapper methods as well?

I guess

  • For get_of_operator_mapper: Yes
  • For get_state_mapper: I think the number of up spins can be determined from the input (occupation numbers). But it is also the case for n_fermions, so I think it's good to add it for consistency.

Copy link
Contributor

@dchung0741 dchung0741 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me overall. Just one slight concern.
Also, about the n_up_spins should I add it in my PR later?

Comment on lines 357 to 365
def augment_dropped_bits(
self,
bit_array: list[int],
n_spin_orbitals: int,
n_fermions: Optional[int] = None,
) -> list[int]:
# Add two qubits dropped by the fermion-to-qubit mapping.
return bit_array + [0, 0]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this method should be public?
My concern is that for SCBK inverse mapping, we do not rearrange the dropped qubit back to the M/2-th and M-th qubit with this method (M is the number of spin orbitals.) Would this cause user's confusions if they decide to use it as a public method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe we can make it private (or you can call it "protected", a term used in other programming languages like C++ or Java).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make it protected in this PR

@toru4838
Copy link
Contributor

toru4838 commented Aug 8, 2023

Thank you for your feedback!

Also, about the n_up_spins should I add it in my PR later?

This PR only considers restricted (OF's) transformations, so we don't need n_up_spins for get_operator_mapper() and get_state_mapper() here. Let's add it in your PR later.

@github-actions github-actions bot temporarily deployed to pull request August 9, 2023 05:12 Inactive
@kwkbtr kwkbtr merged commit 3f5b04a into main Aug 9, 2023
@kwkbtr kwkbtr deleted the inv-state-mapper branch August 9, 2023 06:33
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants