Skip to content

SimulatesIntermediateState._base_iterator docstring says it'll be deprecated #5400

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

Closed
daxfohl opened this issue May 25, 2022 · 2 comments · Fixed by #5402
Closed

SimulatesIntermediateState._base_iterator docstring says it'll be deprecated #5400

daxfohl opened this issue May 25, 2022 · 2 comments · Fixed by #5402
Labels
area/simulation kind/design-issue A conversation around design triage/discuss Needs decision / discussion, bring these up during Cirq Cynque

Comments

@daxfohl
Copy link
Collaborator

daxfohl commented May 25, 2022

I looked into actually deprecating this but I don't think deprecation is the right thing to do here.

I believe SimulatesIntermediateState has too much simulation functionality tied to it; it should just be a wrapper around iterating intermediate states. The rest really belongs in SimulatorBase.

I think in the final solution, _base_iterator should be an abstract function in SimulatesIntermediateState. The implementation of it should be moved to SimulatorBase. Upon moving that, we can get rid of the abstract functions _create_simulation_state, _create_act_on_args, and _core_iterator from SimulatesIntermediateState, since they were only ever used from _base_iterator implementation. This provides better separation of concerns, and allows users more flexibility in implementing SimulatesIntermediateState.

@daxfohl daxfohl added the kind/design-issue A conversation around design label May 25, 2022
@dstrain115 dstrain115 added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque time/before-1.0 labels May 25, 2022
@dstrain115
Copy link
Collaborator

Discussion from cirq cync: This needs a decision before 1.0 since it affects the user-facing interface, but we were unsure whether it should be done without details from @daxfohl . Keeping it on triage discuss for next time.

@daxfohl
Copy link
Collaborator Author

daxfohl commented May 25, 2022

I generally have conflicts at that time but I created a simple PR with the implementation details. @95-martin-orion probably has the most context for a go no-go on it.

CirqBot pushed a commit that referenced this issue Jun 2, 2022
Fixes #5400

Add a deprecation warning that _base_iterator is going to be made abstract, and copy (with intent to move) the implementation to SimulatorBase.

Once the deprecation timeline has passed we can safely make `SimulatesIntermediateState._base_iterator` abstract, and remove `_create_simulation_state`, `_create_act_on_args` and `_core_iterator` declarations from `SimulatesIntermediateState` completely. This makes `SimulatesIntermediateState` more true to its name, as it should not be concerned with how or where the subclass creates the initial state, even from the interface standpoint.

@95-martin-orion
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Fixes quantumlib#5400

Add a deprecation warning that _base_iterator is going to be made abstract, and copy (with intent to move) the implementation to SimulatorBase.

Once the deprecation timeline has passed we can safely make `SimulatesIntermediateState._base_iterator` abstract, and remove `_create_simulation_state`, `_create_act_on_args` and `_core_iterator` declarations from `SimulatesIntermediateState` completely. This makes `SimulatesIntermediateState` more true to its name, as it should not be concerned with how or where the subclass creates the initial state, even from the interface standpoint.

@95-martin-orion
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
Fixes quantumlib#5400

Add a deprecation warning that _base_iterator is going to be made abstract, and copy (with intent to move) the implementation to SimulatorBase.

Once the deprecation timeline has passed we can safely make `SimulatesIntermediateState._base_iterator` abstract, and remove `_create_simulation_state`, `_create_act_on_args` and `_core_iterator` declarations from `SimulatesIntermediateState` completely. This makes `SimulatesIntermediateState` more true to its name, as it should not be concerned with how or where the subclass creates the initial state, even from the interface standpoint.

@95-martin-orion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/simulation kind/design-issue A conversation around design triage/discuss Needs decision / discussion, bring these up during Cirq Cynque
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants