Skip to content

Conversation

@david-pl
Copy link
Collaborator

Fixes #160

@david-pl david-pl requested a review from weinbe58 April 18, 2025 08:51
@codecov
Copy link

codecov bot commented Apr 18, 2025

Codecov Report

Attention: Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/bloqade/pyqrack/qasm2/core.py 85.71% 2 Missing ⚠️
src/bloqade/qasm2/_wrappers.py 85.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
6221 5335 86% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/bloqade/pyqrack/qasm2/core.py 80% 🟢
src/bloqade/qasm2/_wrappers.py 68% 🟢
src/bloqade/qasm2/dialects/core/stmts.py 100% 🟢
TOTAL 83% 🟢

updated for commit: 0b1a721 by action🐍

@cduck
Copy link
Contributor

cduck commented Apr 18, 2025

Is there any type checking concern @Roger-luo with making instructions polymorphic like this?

Copy link
Collaborator

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cduck yes, the type hinting here is not correct. I've commented with a fix above below.

Comment on lines 49 to 51
qarg: ir.SSAValue = info.argument(QubitType | QRegType)
"""qarg (Qubit): The qubit to measure."""
carg: ir.SSAValue = info.argument(BitType)
carg: ir.SSAValue = info.argument(BitType | CRegType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to add a def check_type(self) method to validate the type here because here your definition allows QubitType appear with CRegType.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the type check test. FWIW, mixing the types would have already triggered a RuntimeError in the impl.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to have the error at compile time compared to run time since in some cases you might not run the function with the simulator for awhile if at all.


@wraps(core.Measure)
def measure(qarg: Qubit, cbit: Bit) -> None:
def measure(qarg: Qubit | QReg, cbit: Bit | CReg) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you can fix the type hinting by adding the following separately

from typing import overload

@overload
def measure(qarg: Qubit, cbit: Bit) -> None: ...

@overload
def measure(qarg: QReg, cbit: CReg) -> None: ...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what it looks like in IDE

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Roger-luo I had already done that following your message on slack. I just forgot to push the commit yesterday 😅

@david-pl david-pl requested a review from Roger-luo April 19, 2025 18:59
@david-pl
Copy link
Collaborator Author

david-pl commented Apr 19, 2025

@Roger-luo CI now fails because I added a test for the TypeCheckError that is now raised. However, as of kirin v0.16.8, the resulting error is actually the NoPythonStacktrace base class. Is that on purpose?

Copy link
Member

@weinbe58 weinbe58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the CI is fixed

traits = frozenset({lowering.FromPythonCall()})
qarg: ir.SSAValue = info.argument(QubitType)
qarg: ir.SSAValue = info.argument(QubitType | QRegType)
"""qarg (Qubit): The qubit to measure."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget an out the doc strings!

@Roger-luo
Copy link
Collaborator

right I didn't realize that was actually breaking. I'm cleaning this up in QuEraComputing/kirin#376 so it throws and validation error. We don't have a good way to raise a native exception while hiding python stacktrace at the moment because we we implementing this as custom error handler.

@Roger-luo
Copy link
Collaborator

I figured out a way to raise the exact error that you raise inside the interpreter - it might a bit evil but I think this is cleaner on user side. stay tuned for a new kirin release.

@Roger-luo
Copy link
Collaborator

but I think we should merge this PR given this is due to 0.16.8 actually breaking the convention a bit here. Can you fix the test based on QuEraComputing/kirin#376 first so we can merge this PR but we can cleanup the exception type a bit later?

@weinbe58
Copy link
Member

Mark the test as xfail

@david-pl
Copy link
Collaborator Author

Mark the test as xfail
@weinbe58 the fix from @Roger-luo is already released in v0.16.9, so I guess that comment is no longer relevant?

@david-pl david-pl merged commit 2e9518c into main Apr 22, 2025
11 checks passed
@david-pl david-pl deleted the david/measure-registers branch April 22, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PyQrack simulation backend fails to measure whole registers (only single qubit measurements work)

5 participants