Support cirq emit of rotation when axis is X,Y,Z

[david-pl]

One of the things that dropped out of #436

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
10349	9126	88%	0%	🟢

New Files

No new covered files...

Modified Files

File	Coverage	Status
src/bloqade/squin/cirq/emit/op.py	98%	🟢
src/bloqade/squin/cirq/emit/runtime.py	93%	🟢
TOTAL	96%	🟢

updated for commit: fc5f57d by action🐍

[codecov]

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/bloqade/squin/cirq/emit/op.py	88.23%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[johnzl-777]

Nice! This should fix that problem from the random phase you told me about which causes the occasional failing CI run

[johnzl-777]

Everything looks good, I just want tests to trigger raising the EmitError (I believe codecov has already pointed this out too)

[johnzl-777]

Can you add a not happy path test? To trigger throwing the two EmitErrors I see in emit/op.py?

[david-pl]

Oh damn, this was actually part of #372. Sorry about that, I really need to be more careful with mixing commits.

[david-pl]

Actually, this implementation here is better since it nicely handles some error cases, which the one that was merged doesn't.

@johnzl-777 please review again. I added the tests you asked for, but also slightly changed the implementation: the emit now returns e.g. cirq.Rx rather than cirq.XPowGate, which is the 1:1 mapping we want here, I think.

Sorry about the chaos here 😅

[johnzl-777]

Looks good! I agree with the choice of Rz over the Pow series here, just one thought on my end: is it ever desirable for us to be clever with the translations here or would that be bad UX? As in, lets say somebody requests Rot but the args turn out to be equivalent to a plain old pauli gate.

I imagine there'd be an upside for simulation performance but my concern is it might throw off a user if for some reason they really wanted to preserve Rot as much as possible.

[david-pl]

@johnzl-777 that's a good question! In general, I'd say that whenever you are emitting / lowering you should try to stay as close as possible to a 1:1 mapping. Rewrites such as the one you mention should be other optimization passes afterwards.

When lowering, this would be a rewrite. When emitting, such as in this PR, I'd say leave it to code you are emitting to be smart (or not). For example, it could be that cirq does such rewrites anyway (in which case, fine), or it doesn't. In either case, if we don't introduce any rewrites in the emit, we make sure that we come as close to what a user might expect as possible.

That's just my take on things though ;)