Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate split dtype to join #817

Merged
merged 34 commits into from
Apr 10, 2024

Conversation

fdmalone
Copy link
Collaborator

@fdmalone fdmalone commented Mar 22, 2024

Fixes #722:

Adds back types to joins to ensure no loss of information when a split is followed by a join. I added a severity level to how strictly we may want to enforce type checking:

  1. Strict -> dtype_a == dtype_b
  2. Any -> QAny(bitsize) into other types is ok.
  3. Loose (current): 1 and 2 + some unpleasant integer / fxp casting necessary for phasing via cost function (there's a uint x register and a QFxp register wiring into each other.)

I added an autotester to colorize the check to show how strictly enforced our type checking currently is, and updated the cirq interop to handle the splits and joins there

  • Fix obvious examples.
  • Add autotester to track any finalization type mismatches.
  • cirq to bloq spin / joins

@fdmalone
Copy link
Collaborator Author

Pass == dtype_ is preserved the whole way
Unverified ~ Wiring QAny into A typed register OR casting (Should I add some more colors / categorization @mpharrigan ). It would be nice to have QAny into something colored nicer than Casting
Fail == type checking failed which shouldn't occur since we do this at runtime during the decomposition for egregious type checking failures.

Screenshot 2024-03-22 at 3 43 11 PM

@fdmalone
Copy link
Collaborator Author

fdmalone commented Mar 25, 2024

Post cirq interop split / join fix:
Screenshot 2024-03-25 at 10 28 43 AM

@fdmalone fdmalone marked this pull request as ready for review March 25, 2024 17:52
@fdmalone
Copy link
Collaborator Author

image

@fdmalone fdmalone requested review from mpharrigan and tanujkhattar and removed request for mpharrigan April 3, 2024 23:58
@fdmalone
Copy link
Collaborator Author

fdmalone commented Apr 4, 2024

^^ I'm not actually sure the cirq-interop splits and joins are correct looking and the picture.

@tanujkhattar
Copy link
Collaborator

I'm not actually sure the cirq-interop splits and joins are correct looking and the picture.

@fdmalone What are your concerns? One concern I see is that the QBit() wire going into the x register of pg += x >> 3 should be a QFxp(1, 1) which has a different interpretation than QBit().

I think we default to using QBit() for single bit values but I'm not sure if we should do that always. Definitely not do that here.

@fdmalone
Copy link
Collaborator Author

fdmalone commented Apr 4, 2024

Maybe it's unrelated, I was concerned by the integer-like QFxp(4, 0) splitting into a QFxp(2,2), but I think this fine. I was having some issues with this bloq in classical sim with Fxp but probably unrelated. Ignore my previous comment :)

@fdmalone
Copy link
Collaborator Author

fdmalone commented Apr 6, 2024

@tanujkhattar PTAL

@fdmalone
Copy link
Collaborator Author

fdmalone commented Apr 6, 2024

Safer casting of single qubits into QFxp(1,1) and back to QBit:

Screenshot 2024-04-05 at 5 11 04 PM

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Left a round of comments. Looking good overall. Main question is regarding the special casing for QFxp in _ensure_in_reg_exists

qualtran/cirq_interop/_cirq_to_bloq.py Outdated Show resolved Hide resolved
qualtran/cirq_interop/_cirq_to_bloq.py Outdated Show resolved Hide resolved
qualtran/testing.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

LGTM % one final question

Comment on lines 285 to 290
assert isinstance(
soq.reg.dtype, QBit
), f"Found non-QBit type register which shouldn't happen: {soq.reg.name} {soq.reg.dtype}"
if not isinstance(in_reg.dtype, QBit):
err_msg = (
"Found non-QBit type register which shouldn't happen: "
f"{soq.reg.name} {soq.reg.dtype}"
)
assert isinstance(soq.reg.dtype, QBit), err_msg
Copy link
Collaborator

Choose a reason for hiding this comment

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

The assertion probably does belon above since in both the if / else cases we assume that soq.reg.dtype is of type QBit ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted

@tanujkhattar tanujkhattar enabled auto-merge (squash) April 10, 2024 22:26
@tanujkhattar tanujkhattar merged commit d516599 into quantumlib:main Apr 10, 2024
6 checks passed
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.

[data types] bb./splits/joins/... should preserve dtypes.
2 participants