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

Make state vector copying explicit #3494

Closed
95-martin-orion opened this issue Nov 11, 2020 · 7 comments · Fixed by #5685
Closed

Make state vector copying explicit #3494

95-martin-orion opened this issue Nov 11, 2020 · 7 comments · Fixed by #5685
Assignees
Labels
area/debugging area/performance area/simulation complexity/low introduces/modifies 1-2 concepts, should take 1-2 days max for an advanced contributor kind/health For CI/testing/release process/refactoring/technical debt items skill-level/beginner No background required, self-contained issue triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@95-martin-orion
Copy link
Collaborator

The state_vector method in StateVectorTrialResult includes a copying of the state vector to resolve issue #3101:

return self._final_simulator_state.state_vector.copy()

This eliminates the issue of mutating the state vector, but can significantly increase runtime for large circuits without a clear indication of the root cause. We should force users to explicitly make a copy of the state if they need it, either by renaming the method to state_vector_copy or by removing it altogether and requiring users to copy the final_state_vector field.

Related: qsim issue 237

@95-martin-orion 95-martin-orion added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque kind/health For CI/testing/release process/refactoring/technical debt items area/simulation complexity/low introduces/modifies 1-2 concepts, should take 1-2 days max for an advanced contributor area/performance skill-level/beginner No background required, self-contained issue area/debugging labels Nov 11, 2020
@dabacon
Copy link
Collaborator

dabacon commented Nov 16, 2020

For the final simulator state it's not clear why it needs to copy at all (Think @95-martin-orion commented on this in original PR not sure why I ignored it).

@95-martin-orion 95-martin-orion self-assigned this Dec 2, 2020
@balopat
Copy link
Contributor

balopat commented Jan 6, 2021

ref #3115

@balopat
Copy link
Contributor

balopat commented Jan 6, 2021

Discussed at Cirq Cynque: 1. let's go without copy. 2. make a note in the release notes about potential breakage here.

@balopat balopat added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Jan 27, 2021
@tanujkhattar
Copy link
Collaborator

From cirq sync:

Let's do it!!

@daxfohl
Copy link
Collaborator

daxfohl commented Jun 7, 2022

@95-martin-orion I just remembered to mention: I noticed an unnecessary sv copy introduced in https://github.com/quantumlib/Cirq/pull/4533/files. While that PR made sweeps more efficient, it inordinately makes an unnecessary copy of the state when running simulate: simulate calls into this new function, which creates the SV and calls the base implementation. The changes to the base implementation in that same PR then copies the state for each paramresolver (which in this case there's only one) and runs on it. Thus we end up with two copies of the initial state vector when we only needed one.

One solution is to have the base implementation skip copying the state for the last paramresolver, and just use the provided state directly. Officially this would be breaking since simulate_sweep_iter is public.

@95-martin-orion
Copy link
Collaborator Author

I just remembered to mention: I noticed an unnecessary sv copy introduced in #4533...

Kind of tangential to this issue, but I support the solution you've described. What happened to initial_state prior to your changes? It looks like it was consumed (and possibly mutated?) by simulate_moment_steps, but that conflicts with it being reused for every parameterization...

CirqBot pushed a commit that referenced this issue Jun 8, 2022
Removes the copy when simulating params. #3494 (comment) for discussion. @95-martin-orion
CirqBot pushed a commit that referenced this issue Jun 10, 2022
Part of #3494 (requires deprecation cycle to resolve).

This adds the `copy` parameter to all `state_vector` methods affected by #3494, and sets up deprecation messages to change the default copy behavior to "don't copy" in the next Cirq release.

The only substantive changes are in `sparse_simulator.py`, `state_vector.py`, and `state_vector_simulator.py`; all other changes simply inject `copy` to prevent deprecation warnings from `state_vector` calls (True for step results, False for final results).
@95-martin-orion
Copy link
Collaborator Author

With #5324 merged, all that's left for this is to complete the deprecation and switch the default to copy=False.

CirqBot pushed a commit that referenced this issue Jul 11, 2022
Fixes #3494.

This PR bundles together a bunch of cleanup around `[final_]state_vector` deprecations.
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Part of quantumlib#3494 (requires deprecation cycle to resolve).

This adds the `copy` parameter to all `state_vector` methods affected by quantumlib#3494, and sets up deprecation messages to change the default copy behavior to "don't copy" in the next Cirq release.

The only substantive changes are in `sparse_simulator.py`, `state_vector.py`, and `state_vector_simulator.py`; all other changes simply inject `copy` to prevent deprecation warnings from `state_vector` calls (True for step results, False for final results).
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Fixes quantumlib#3494.

This PR bundles together a bunch of cleanup around `[final_]state_vector` deprecations.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
Removes the copy when simulating params. quantumlib#3494 (comment) for discussion. @95-martin-orion
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
Part of quantumlib#3494 (requires deprecation cycle to resolve).

This adds the `copy` parameter to all `state_vector` methods affected by quantumlib#3494, and sets up deprecation messages to change the default copy behavior to "don't copy" in the next Cirq release.

The only substantive changes are in `sparse_simulator.py`, `state_vector.py`, and `state_vector_simulator.py`; all other changes simply inject `copy` to prevent deprecation warnings from `state_vector` calls (True for step results, False for final results).
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
Fixes quantumlib#3494.

This PR bundles together a bunch of cleanup around `[final_]state_vector` deprecations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/debugging area/performance area/simulation complexity/low introduces/modifies 1-2 concepts, should take 1-2 days max for an advanced contributor kind/health For CI/testing/release process/refactoring/technical debt items skill-level/beginner No background required, self-contained issue triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants