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

str/repr blows up for large disjoint states #4477

Closed
daxfohl opened this issue Aug 28, 2021 · 4 comments · Fixed by #4518
Closed

str/repr blows up for large disjoint states #4477

daxfohl opened this issue Aug 28, 2021 · 4 comments · Fixed by #4518
Labels
area/simulation kind/bug-report Something doesn't seem to work. needs agreed design We want to do this, but it needs an agreed upon design before implementation priority/p1-urgent Fix is needed as soon as possible. Should be staffed. It is blocking some major flows for users triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@daxfohl
Copy link
Contributor

daxfohl commented Aug 28, 2021

Description of the issue

The repr and str functions of TrialResult and subclasses all compute the full state of the system as a kron of all disjoint states before displaying. In large disjoint states, this causes the system to hang. This can crop up in unusual places like assertions and stack traces. See #4360 (comment).

How to reproduce the issue

In the density_matrix_simulator_test::test_large_untangled_okay unit test, change the number of qubits to mismatch. The simulation works fine, but the test then hangs at assert set(result._final_step_result.qubits) == set(cirq.LineQubit.range(59)), because it tries to calculate the repr of result to display. This led to users thinking that the simulation itself was hanging, which was not the case.

I propose we update these to display the individual state spaces and associated qubits, rather than joining into a single space prior to output.

@daxfohl daxfohl added the kind/bug-report Something doesn't seem to work. label Aug 28, 2021
@95-martin-orion 95-martin-orion added area/simulation triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Sep 16, 2021
@dstrain115 dstrain115 added triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add needs agreed design We want to do this, but it needs an agreed upon design before implementation priority/p1-urgent Fix is needed as soon as possible. Should be staffed. It is blocking some major flows for users and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Sep 22, 2021
@dstrain115
Copy link
Collaborator

Discussion from cirq cync: This seems bad. We should fix this fairly urgently, but we don't have a solution yet or someone assigned.

@daxfohl
Copy link
Contributor Author

daxfohl commented Sep 22, 2021

I can take this. It's related to the state separation functionality introduced in #4100. Updating the code to output these as a list of independent state vectors or density matrices should be straightforward.

I think as part of this we should also add a public function to query for the independent state space that contains a qubit too. (e.g. trial_result.get_state(q1).qubits and trial_result.get_state(q1).density_matrix would return the qubits and density matrix for whatever state space q1 is in). Currently that's all private as an implementation detail, but if we're updating the str/repr to display this information, then it seems logical that we should have an easy way for users to get that raw data too. @95-martin-orion @MichaelBroughton thoughts?

@95-martin-orion
Copy link
Collaborator

I think as part of this we should also add a public function to query for the independent state space that contains a qubit too.

This sounds like a reasonable, low-cost addition to me - though I would prefer get_state_containing_qubit to be clear about what the method is doing. (e.g. to avoid confusion that a state is only associated with the provided qubits)

@daxfohl
Copy link
Contributor Author

daxfohl commented Sep 23, 2021

Updated linked PR with this function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/simulation kind/bug-report Something doesn't seem to work. needs agreed design We want to do this, but it needs an agreed upon design before implementation priority/p1-urgent Fix is needed as soon as possible. Should be staffed. It is blocking some major flows for users triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants