[XEB] Optimize/characterize by pair #3795
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
One thing which is beyond this PR but might be a concern is that it's very hard to review if the code operating on DataFrames is correct. The dataframes have a lot of context which is almost impossible to learn from the code itself.
One possibility to ease this might be having more verbose/explicit tests with hard-coded input and output dataframes.
| assert len(row['fidelities']) == len(cycle_depths) | ||
|
|
||
|
|
||
| def test_benchmark_2q_xeb_fidelities_vectorized(): |
There was a problem hiding this comment.
Would it be possible to have a test with hard-coded expected values of all the intermediate variables instead of realying on two different implementations of the same logic?
There was a problem hiding this comment.
that seems more fragile, no? having two independent implementations is the best possible unit test!
There was a problem hiding this comment.
How do you test if the other implementation doesn't have a bug then?
There was a problem hiding this comment.
there are other tests that test the whole function, which the previous and current one is passing. This is an additional test to make sure the two implementations are identical.
|
I consider the comments minor and I'll LGTM it now. |
|
Re dataframes and testing experimental/workflow code in general: I think the notebooks (which thanks to @balopat are now run against cirq master on each PR!) are super helpful for making sure the whole flow works as expected. You can already see in the "unit" tests that I have to basically mock out a whole experiment to test a given part. This is expected from the more "workflow-type" functionality in this suite of PRs. The unit behavior is simple and tested elsewhere (generating random circuits, simulating them, executing them ... this is all covered well in cirq!) |
mpharrigan
force-pushed
the
2021-02-xeb-3.6
branch
from
283ead9 to
69407b5
Compare
|
PTAL (I know you already approved, but I'll give you a chance to respond to my responses anyways) |
| assert len(row['fidelities']) == len(cycle_depths) | ||
|
|
||
|
|
||
| def test_benchmark_2q_xeb_fidelities_vectorized(): |
There was a problem hiding this comment.
How do you test if the other implementation doesn't have a bug then?
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
There was a problem hiding this comment.
CODEOWNERS approval for docs/ files, for which @mrwojtek is not an owner.
CirqBot
removed
automerge
Add a new method to perform optimization of angles (aka characterization) by pair. This is essential for "parallel" XEB. At it's heart, this is simply a "groupby" operation on the dataframe before calling the characterize function. There are additional code changes:
apply, which turned out to be very slow! I profiled thebenchmarkfunction on a test workflow. It was taking 7.87 seconds, of which 5.3s were cumulative inside the apply function. The rest of it involved simulating the many circuits.benchmark_...is called during the optimization's objective function so it's performance critical. With the change, the call was a total of 2.5s and the least-squares estimation no longer appeared in the top many cumulative-time profiling calls.characterize_..function has been beefed up to be a dataclass with more fields. Not only do you get the optimization results, but a nicer dictionary of angles and a dataframe of the refit fidelities. This simplifies one of the older notebooks. This data is also per-pair so you can do parallel xeb effectivelyexperiments.cross_entropy_benchmarking, which will be deprecated, [XEB] Plan to deprecate duplicate functionality #3775