Speed up tests using mocks #2126
Conversation
this function always gets called with a single PauliString object, and it wouldn't make sense to call with a list/sequence
these two tests do not need to run any of the computation done in the `test_execute_with_qse` since that test already covers that! no need to check performance either as, again, the previous test covers that. these tests exist only to ensure the two alternate workflows run.
seems very strange to me to have it defined after the function that uses it. It's valid, but harder to understand IMO.
mocking this unearthed a bug with the cost estimator. Since for each problem/circuit, we compute the noisy value in addition to the EM, we were actually undercounting the estimation by `num_circuits`
closes #2015 (discussion)
natestemen
added
the
infrastructure
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2126 +/- ##
==========================================
- Coverage 98.21% 98.21% -0.01%
==========================================
Files 87 87
Lines 4150 4140 -10
==========================================
- Hits 4076 4066 -10
Misses 74 74 ☔ View full report in Codecov by Sentry. |
|
|
||
|
|
||
| def _compute_overlap_matrix( | ||
| circuit: QPROGRAM, | ||
| executor: Union[Executor, Callable[[QPROGRAM], QuantumResult]], | ||
| check_operators: Sequence[PauliString], | ||
| pauli_string_to_expectation_cache: Dict[PauliString, complex] = {}, | ||
| pauli_expectation_cache: Dict[PauliString, complex] = {}, | ||
| code_hamiltonian: Optional[Observable] = None, |
There was a problem hiding this comment.
Is this new argument here in order to get rid of _compute_hamiltonian_overlap_matrix ?
Seems good, just trying to understand.
There was a problem hiding this comment.
Yup that's exactly right! We previously had
_compute_hamiltonian_overlap_matrix, and_compute_overlap_matrix
which were largely the same code, except for one line. I combined the two functions in order to cut down on repeated code.
| noise_model_function=cirq.depolarize, | ||
| noise_level=(0.01,), | ||
| ) | ||
| assert mock_execute_with_qse.call_count == num_circuits |
There was a problem hiding this comment.
Do you know how mock_execute_with_qse.call_count "knows" that it should return num_circuits?
All seems to work. I am just curious how unittest.mock.patch is able to mock functions without explicitly setting return values.
There was a problem hiding this comment.
I was also surprised you don't have to set a return value, but it's nice not having to set some random value for it since it's meaningless (in this instance).
As for how it actually works, unittest.mock.patch creates a MagicMock instance for the execute_with_qse function. Under the hood my guess is that the MagicMock class looks something like this:
class MagicMock:
def __init__(self):
self.call_count = 0
def __call__(self):
self.call_count += 1
Description
Speed up the tests round three. This time mostly using the mocking approach in order to cut down on the expensive computations that are performed. I also cleaned up some of the adjacent code while trying to understand the QSE tests.
The following table is the runtime of
make testbefore and after the changes included in this PR.fixes #2080