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

[data types] Enforce type checking #710

Merged
merged 17 commits into from
Mar 11, 2024

Conversation

fdmalone
Copy link
Collaborator

@fdmalone fdmalone commented Feb 23, 2024

  1. Adds a check in _process_soquests that the register dtypes are "consistent".
  2. Adds a add_register_from_dtype method.
  3. Adds an QAnyInt and QAnyUInt bundle of types.

The type checking raised a bug in the THC bloq with register mismatch which is a good sign. I fixed that too (although, still not entirely satisfied with our grouping for Select and Prepare #549)

The check in process soquets checks both the type and the register size. The register size was not being checked before so this sort of deprecates the assert_connections_compatible check (I deleted that unit test).

The current type consistency is a bit sketchy:

  1. type_a == type_b (fine)
  2. type_a or type_b QAny (fine)
  3. Single bit registers are ok (fine)
  4. Integer types of same bitsize are ok (this is probably ok, but maybe we should force people to cast)
  5. Unsigned integers types are ok with wholly fractional or wholly integer QFxp. (this is a bit sketchy)

Maybe 5 and 6 should go away and we should add casts (#725)? This runs the risk of making the code more verbose.

@fdmalone fdmalone marked this pull request as ready for review February 29, 2024 01:27
@fdmalone fdmalone marked this pull request as draft February 29, 2024 19:04
@fdmalone fdmalone requested review from tanujkhattar and mpharrigan and removed request for tanujkhattar February 29, 2024 20:06
@fdmalone fdmalone marked this pull request as ready for review February 29, 2024 20:08
@fdmalone
Copy link
Collaborator Author

fdmalone commented Mar 5, 2024

The test failure seems unrelated.

@mpharrigan
Copy link
Collaborator

Maybe 5 and 6 should go away

can you clarify this? your numbered list only goes up to 5


I think I'm most worried about

Single bit registers are ok (fine)

mainly based around the discussion about whether you can have one-bit number types (like a 1 bit unsigned integer vs a 1 bit twos-compliment signed integer). I forget: do we allow these? Even if we don't: there were some convincing arguments made in their favor. In this world: a one-bit unsigned integer is basically the same as QBit() but a one-bit twos-compliment signed integer represents the values 0 and -1 rather than 0, 1 so it's not the same

@mpharrigan
Copy link
Collaborator

The check in process soquets checks both the type and the register size. The register size was not being checked before so this sort of deprecates the assert_connections_compatible check

I'm confused about this. My technical confusion is that you deleted the unit test but you didn't delete the testing function. Did you mean to delete both?

Philosophically: the qualtran.testing.assert_xxx generally are redundant with the bloq builder checks. The motivation for this is two-fold: 1) check CompositeBloq that wasn't constructed via the bloqbuilder. 2) there's a distinct possibility that the BloqBuilder checks will have to be paired back due to performance or annoyance/debugging reasons. Right now the door is still open to a world where validation is separate from constructing the object.

@fdmalone
Copy link
Collaborator Author

fdmalone commented Mar 6, 2024

Maybe 5 and 6 should go away

can you clarify this? your numbered list only goes up to 5

Sorry, 4 and 5 (reinterpreting essentially) which is related to your 1-bit concern below. Should we allow the free reinterpretation of an unisgned int to a signed int under the assumption the developer know's what they're doing? In the classical case perhaps we don't care about the value in the register and just want to reuse the "variable" / qubits for something else so assume things will be appropriately cleared before use.

I think I'm most worried about

Single bit registers are ok (fine)

mainly based around the discussion about whether you can have one-bit number types (like a 1 bit unsigned integer vs a 1 bit twos-compliment signed integer). I forget: do we allow these? Even if we don't: there were some convincing arguments made in their favor. In this world: a one-bit unsigned integer is basically the same as QBit() but a one-bit twos-compliment signed integer represents the values 0 and -1 rather than 0, 1 so it's not the same

We do allow the one bit integers as you described. I think this comparison is related more generally to the point above about integers. My sense is we shouldn't be too strict to avoid adding a bunch of Cast boilerplate, but I could be convinced.

@fdmalone
Copy link
Collaborator Author

fdmalone commented Mar 6, 2024

The check in process soquets checks both the type and the register size. The register size was not being checked before so this sort of deprecates the assert_connections_compatible check

I'm confused about this. My technical confusion is that you deleted the unit test but you didn't delete the testing function. Did you mean to delete both?

Philosophically: the qualtran.testing.assert_xxx generally are redundant with the bloq builder checks. The motivation for this is two-fold: 1) check CompositeBloq that wasn't constructed via the bloqbuilder. 2) there's a distinct possibility that the BloqBuilder checks will have to be paired back due to performance or annoyance/debugging reasons. Right now the door is still open to a world where validation is separate from constructing the object.

I deleted the test because the error raised was from the type check in process soquets. I wasn't sure what the right way to bypass that check was and was unsure if the function was useful elsewhere? Given your comment it seems like the correct way to test this method would be to not use the BloqBuilder or maybe the test is now redundant?

@mpharrigan
Copy link
Collaborator

I deleted the test because the error raised was from the type check in process soquets

Ah, I understand now. The other unit tests for the testing functions (head spin) use the helper function _manually_make_test_cbloq_cxns because otherwise the BloqBuilder would raise an exception before you could even use the assert_xxx testing functions. This is what you ran into. test_assert_connections_compatible is unique because the BloqBuilder was doing less (none?) validation on whether connections were compatible vs qlt_testing.assert_connections_compatible. The full fix is

  1. update qlt_testing.assert_connections_compatible to call your new datatype validation function
  2. re-write the unit test starting from _manually_make_test_cbloq_connections and muck them up a little to trigger the validation failure

@fdmalone fdmalone mentioned this pull request Mar 6, 2024
@fdmalone
Copy link
Collaborator Author

@mpharrigan is this good to go?

@fdmalone fdmalone enabled auto-merge (squash) March 11, 2024 18:34
@fdmalone fdmalone merged commit 1715243 into quantumlib:main Mar 11, 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.

2 participants