Conversation
476259c to
b8ff0f0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors SoquetT from a Union to a typing.Protocol to better support duck typing. This is a significant improvement that makes the type system more flexible and expressive. The changes throughout the codebase to adopt the new SoquetT protocol and the BloqBuilder.is_single/is_ndarray type guards are well-executed. My review includes a couple of suggestions to improve type consistency in the BloqBuilder's free and split methods to fully align with the new protocol-based approach.
| @@ -1395,10 +1430,10 @@ def allocate( | |||
| def free(self, soq: Soquet, dirty: bool = False) -> None: | |||
There was a problem hiding this comment.
The type hint for soq is Soquet, but the implementation with BloqBuilder.is_single(soq) suggests it should handle SoquetT. This is inconsistent. To align with the goals of this PR and for type correctness, the type hint for soq should be SoquetT. This will allow type checkers to correctly analyze code that passes SoquetT objects to this method.
| def free(self, soq: Soquet, dirty: bool = False) -> None: | |
| def free(self, soq: SoquetT, dirty: bool = False) -> None: |
There was a problem hiding this comment.
Can I talk back to you? I consider the type annotations to be a form of documentation and certainly not infallible. This is supposed to be a single soquet, but the code will behave unpredictably if passed something else. The documented type annotation is what's "supposed" to happen, but the runtime code handles what may actually happen.
Switches the
SoquetTunion type alias to atyping.Protocol. This supports "duck typing", which is appropriate for the situation whereSoquetTis used as a type annotation.Specifically,
SoquetTis now anything that has.shapeand.item(*args). With the addition of these properties/methods to theSoquetclass, now this is satisfied bySoquetandNDArray. Note: we can't use mypy to annotate things withNDArray[Soquet]now or before due to limitations in numpy.When users or developers need to dispatch depending on whether a
SoquetTis a single soquet or an array thereof, they should use the newBloqBuildermethods. The example from the new docstring:In the protocol implementations and bloq standard library, this PR:
Soquet_infra/and protocols modules. Following this, pytest passes but there are mypy issues in thebloqs/standard librarybloqs/standard library.uses new alias(removed from this PR)soq.dtypeinstead ofsoq.reg.dtype.Regarding #1720: this PR shows the changes to the standard library that would be required to avoid
isinstance(x, Soquet)checks