-
Notifications
You must be signed in to change notification settings - Fork 982
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
Density matrix simulator EVs #3979
Conversation
Assigning to @balopat (the next triage expert) for review, since I can't review my own PRs 😞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to reduce the duplication introduced here.
1.) production code: I would experiment with pulling out the repeated lines and have a new "protected" method expectation_value_from_result(obs: PauliSum, result:TSimulationResult) -> float
which then can be called from the abstract method
2.) test code: I would move factor out the expectation value calculation logic to a separate test.py file and parameterized it with the different simulator types.
Packing the duplicated code in I stand by my top-of-PR comment: the correct way to de-dupe this would be something closer to #3990. (Probably the cleanest solution would involve converting |
Ah, cool, I missed that, and oh, nice, yes that is exactly the direction I was thinking - having an Also, note that #3990 adds yet another test with exactly the same cases at a first blink...maybe we should think about extracting a I'm okay to get this merged but I would love to have an issue created for deduping both impl and tests for this and referenced in a comment in all these places.
This sounds interesting but I just don't see why yet, can you elaborate on this idea? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - given we add an issue and a comment for deduping tests & design
ping @95-martin-orion we could merge this, just add in a comment so we don't forget to dedupe |
Opened #4209 to track deduplication. |
Fixes quantumlib#3964. CC @zaqqwerty This is a near-exact copy of the behavior and tests for `sparse_simulator`. If we see the need in the future, this code could probably be folded into a `SimulatesEVsAndFullState` interface as the default `simulate_expectation_values_sweep` behavior, but that doesn't seem particularly urgent at the moment. Relevant changes from the `sparse_simulator` code: - `obs.expectation_from_density_matrix(result.final_density_matrix, qmap)` instead of state-vector equivalent - added `test_simulate_noisy_expectation_values` test - that's it
Fixes #3964. CC @zaqqwerty
This is a near-exact copy of the behavior and tests for
sparse_simulator
. If we see the need in the future, this code could probably be folded into aSimulatesEVsAndFullState
interface as the defaultsimulate_expectation_values_sweep
behavior, but that doesn't seem particularly urgent at the moment.Relevant changes from the
sparse_simulator
code:obs.expectation_from_density_matrix(result.final_density_matrix, qmap)
instead of state-vector equivalenttest_simulate_noisy_expectation_values
test