Refactor DFE to run samplers #2870
Conversation
|
|
||
| Returns: | ||
| The estimated fidelity. | ||
| """ | ||
| # n_trials is upper-case N in https://arxiv.org/abs/1104.3835 | ||
|
|
||
| if log_output_filename: | ||
| sys.stdout = open(log_output_filename, "w") |
There was a problem hiding this comment.
This breaks the principle of separation of concerns. Function for direct fidelity estimation should not be responsible for setting up logging.
Perhaps we don't really need to print anything in this function. It seems that what we print are the inputs and return value anyway.
There was a problem hiding this comment.
I had a concern that it might not be the right way. I reverted the changes to the logging, and instead narrowed the scope to only the refactoring and I will address the logging later?
|
Thanks. I don't think I have permissions to merge so I'll wait for the bot :) |
|
Thanks. PTAL. |
| fidelity = 0.0 | ||
|
|
||
| if samples_per_term == 0: | ||
| # sigma in https://arxiv.org/abs/1104.3835 | ||
| assert type(sampler) == cirq.DensityMatrixSimulator |
There was a problem hiding this comment.
Please raise an exception instead of asserting.
There was a problem hiding this comment.
Like this?
| 'noise' parameter above and instead run an estimation of the | ||
| characteristic function. | ||
|
|
||
| samples_per_term: is set to 0, we use the 'sampler' parameter above as |
There was a problem hiding this comment.
nit: replace "is set to 0" with "if set to 0"
|
Sorry about this. I think I hadn't pushed on time. I checked that the changes are in. Thank you for the careful review. PTAL, no hurry. |
| fidelity = 0.0 | ||
|
|
||
| if samples_per_term == 0: | ||
| # sigma in https://arxiv.org/abs/1104.3835 | ||
| if not isinstance(sampler, cirq.DensityMatrixSimulator): | ||
| raise TypeError('sampler is not a cirq.DensityMatrixSimulator') |
There was a problem hiding this comment.
nit: maybe add "..., but samples_per_term is zero" to guide the user towards the correct parameters.
| fidelity = 0.0 | ||
|
|
||
| if samples_per_term == 0: | ||
| # sigma in https://arxiv.org/abs/1104.3835 | ||
| if not isinstance(sampler, cirq.DensityMatrixSimulator): | ||
| raise TypeError('sampler is not a cirq.DensityMatrixSimulator') |
There was a problem hiding this comment.
Test coverage failed. You need a unit test that verifies the exception is raised in the right circumstances.
viathor
changed the title
|
Sorry for the double-push. My first one failed the check, so I was waiting to comment but now I can because the second push passed. PTAL. |
viathor
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
No description provided.