refactor[next]: new type system #1428
Conversation
| import dataclasses | ||
|
|
||
|
|
||
| class Trait: |
There was a problem hiding this comment.
Since this class is empty it can only be used for type annotations or isinstance() checks, so it works as a Union type, which python already defines and supports exactly these two cases. I would remove the empty class and add the explicit union type at the end of the file.
Trait = SignednessTrait | FromTrait | ...
There was a problem hiding this comment.
The issue with this approach is that if you create a new SomethingTrait outside of this file, you won't be able to make it a subclass of Trait for the annotations and isinstance checks.
| class SignednessTrait(Trait): | ||
| """Specifies the signedness of a type.""" | ||
| @abc.abstractmethod | ||
| def is_signed(self) -> bool: | ||
| ... |
There was a problem hiding this comment.
An abstractmethod can only be used inside a class inheriting from abc.ABC, otherwise it's useless. In this case, since the abstract class is only defining and interface and not implementing any common functionality, it would make even more sense to define the class as a runtime checkable protocol (which is also an ABC class), and thus inheriting from it would be completely optional.
| class SignednessTrait(Trait): | |
| """Specifies the signedness of a type.""" | |
| @abc.abstractmethod | |
| def is_signed(self) -> bool: | |
| ... | |
| @runtime_checkable | |
| class SignednessTrait(Protocol): | |
| """Specifies the signedness of a type.""" | |
| @abc.abstractmethod | |
| def is_signed(self) -> bool: | |
| ... |
The same comment applies to all Trait classes in this file.
There was a problem hiding this comment.
Good catch. I added the ABC as base class for all of them that had abstract methods.
I don't think this is very well suited for protocols though. Protocols help you decide if an object fits a well-known interface after you've made the object, while abstract base classes help you create an object after you decided it fits a well-known concept. Signedness is a well-known concept, but there is no well-known interface for it in this context, hence the abstract base class that defines the interface.
IMO it's also useful that you can look at a Type and tell at a glance what Traits it implements just from the list of base classes instead of having to go through its methods and matching them against all the different Traits. There is also the possibility of a Trait with no methods, in which case you couldn't use runtime checkable.
| from .traits import FunctionArgument | ||
|
|
||
|
|
||
| class Type: |
There was a problem hiding this comment.
This class is not needed. See my comment in traits.Trait.
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| class IntegerType( |
There was a problem hiding this comment.
These IntegerType and FloatType definitions make sense to me. However, the concepts here deviate a bit from the most common conventions in array/tensor libraries used in the community. Ideally, we could represent arithmetic types in a way that is compatible with both dtype-like descriptors used by array libraries at runtime for data exchange and allocation, and internally in our description of the ASTs. A first step into this direction are the DType definitions on gt4py._core.definitions based on DLpack (https://dmlc.github.io/dlpack/latest/) and the Python Array API standard (https://data-apis.org/array-api/latest/API_specification/type_promotion.html#type-promotion). The current implementation there still needs to be polished and extended so I think it would make sense to try to unify those with the definitions here, instead of having two similar but incompatible representations for the same concept.
Actually, I think the core part of the new_type_system could be moved into gt4py._core to make things simpler.
There was a problem hiding this comment.
I don't really think it's a good idea to include these tasks in this PR. First, the goal of this PR was to find a cleaner representation of AST types, and it delivers that in a self-contained package. Turning it into an open-ended task would just delay it and reduce quality. Second, if the DType definitions and their use cases and usage patterns are not clear, it's difficult to decide if merging makes sense at all, and if it does, how it should be done. And, third, this type system is only for gt4py.next, so unless it's backported to the cartesian version, I don't see the benefits of lifting it out of next.
There was a problem hiding this comment.
In the future, yes, it might make sense to do these changes, but if the code is kept clean and well-tested such refactorings should be fairly straightforward.
No description provided.