Two stage ZNE #2452
Two stage ZNE #2452
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2452 +/- ##
=======================================
Coverage 98.70% 98.70%
=======================================
Files 88 88
Lines 4083 4091 +8
=======================================
+ Hits 4030 4038 +8
Misses 53 53 ☔ View full report in Codecov by Sentry. |
mitiq/zne/tests/test_zne.py
Outdated
| @pytest.mark.parametrize( | ||
| "extrapolation_factory", [RichardsonFactory, LinearFactory] | ||
| ) | ||
| def test_two_stage_zne(noise_scaling_method, extrapolation_factory): |
There was a problem hiding this comment.
This test does a good job of testing the combined functionality, but it would be good to have individual unit tests for each of the functions separately. WDYT? Would it be redundant?
There was a problem hiding this comment.
I think for now the combined test is good enough. As we iterate in future PRs and the functions get more complicated, it might be better to separate them then.
remove comment in test_zne.py Co-authored-by: nate stemen <[email protected]>
use Sequence instead of list in typehinting for scale_factors Co-authored-by: nate stemen <[email protected]>
mitiq/zne/tests/test_zne.py
Outdated
| if to_frontend == to_braket: | ||
| assert all(isinstance(circ, braket.circuits.Circuit) for circ in circs) | ||
| elif to_frontend == to_qiskit: | ||
| assert all(isinstance(circ, qiskit.QuantumCircuit) for circ in circs) | ||
| elif to_frontend == to_pennylane: | ||
| assert all( | ||
| isinstance(circ, pennylane.tape.QuantumTape) for circ in circs | ||
| ) | ||
| elif to_frontend == to_pyquil: | ||
| assert all(isinstance(circ, pyquil.quil.Program) for circ in circs) | ||
| elif to_frontend == to_qibo: | ||
| assert all(isinstance(circ, qibo.Circuit) for circ in circs) | ||
| else: | ||
| assert all(isinstance(circ, cirq.Circuit) for circ in circs) |
There was a problem hiding this comment.
These tests are redundant since we already know our scaling method functions preserve input type. E.g. fold_global never converts a qibo.Circuit to anything but a qibo.Circuit. Since those tests cover this, let's leave it out from here for readability.
There was a problem hiding this comment.
I see, that makes sense. I'll remove these tests!
| ) | ||
| @pytest.mark.parametrize( | ||
| "to_frontend", | ||
| [None, to_qiskit, to_braket, to_pennylane, to_pyquil, to_qibo], |
There was a problem hiding this comment.
| [None, to_qiskit, to_braket, to_pennylane, to_pyquil, to_qibo], | |
| [convert_to_mitiq, to_qiskit, to_braket, to_pennylane, to_pyquil, to_qibo], |
I think we can use this instead to leave the circuit as a cirq.Circuit object instead of doing the check below.
There was a problem hiding this comment.
That makes it a bit cleaner indeed!
There was a problem hiding this comment.
Actually, convert_to_mitiq() returns a tuple (the circuit and a string). So you still need one check right (if to_frontend = convert_to_mitiq)?
mitiq/zne/zne.py
Outdated
| def combine_results( | ||
| scale_factors: Sequence[float], | ||
| results: list[float], | ||
| extrapolation_method: Callable[[list[float], list[float]], float], |
There was a problem hiding this comment.
Might want to generalize these to Sequence[float] to make mypy happy.
|
Well done on having this in @FarLab. Later on it could be nice to have very short docs on the added functionality for modularization, maybe once we have more in other techniques, but we can also start with ZNE only. |
| circuits = [] | ||
| for scale_factor in scale_factors: | ||
| circuits.append(scale_method(circuit, scale_factor)) | ||
|
|
||
| return circuits |
There was a problem hiding this comment.
this should be a list comprehension fo sho
return [scale_method(circuit, s) for s in scale_factors]
| res = extrapolation_method(scale_factors, results) | ||
|
|
||
| return res |
There was a problem hiding this comment.
unnecessary variable
return extrapolation_method(scale_factors, results)
| "to_frontend", | ||
| [None, to_qiskit, to_braket, to_pennylane, to_pyquil, to_qibo], | ||
| ) | ||
| def test_two_stage_zne( |
There was a problem hiding this comment.
There should be two separete unit tests for the two new functions you implemented. This is an integration test, and one bad symptom is that this test is violating the AAA pattern.
This PR modularizes ZNE using the design described in #2439.