feat[next]: Embedded domain construction from dimension comparison#2532
feat[next]: Embedded domain construction from dimension comparison#2532havogt merged 9 commits intoGridTools:mainfrom
Conversation
egparedes
left a comment
egparedes
left a comment
There was a problem hiding this comment.
Just a couple of questions.
docs/development/ADRs/next/0022-Limitations-of-embedded-concat_where.md
Outdated
Show resolved
Hide resolved
src/gt4py/next/common.py
Outdated
| @overload | ||
| def __eq__(self, value: core_defs.IntegralScalar) -> Domain: ... # type: ignore[overload-overlap] # intentionally returns Domain, not bool | ||
| @overload | ||
| def __eq__(self, value: object) -> bool: ... |
There was a problem hiding this comment.
I'm not sure if this works, but IIUC, this is the implemented behavior. If this works, a similar change could be applied to __ne__
| def __eq__(self, value: object) -> bool: ... | |
| def __eq__(self, value: object) -> Literal[False]: ... |
There was a problem hiding this comment.
mypy doesn't like it: Overloaded function signatures 1 and 3 overlap with incompatible return types
egparedes
left a comment
egparedes
left a comment
There was a problem hiding this comment.
LGTM. I'm approving the PR but have a couple of comments.
src/gt4py/next/common.py
Outdated
| @overload | ||
| def __eq__(self, value: Dimension) -> bool: ... | ||
| @overload | ||
| def __eq__(self, value: core_defs.IntegralScalar) -> Domain: ... # type: ignore[overload-overlap] # intentionally returns Domain, not bool | ||
| @overload | ||
| def __eq__(self, value: object) -> bool: ... | ||
| def __eq__(self, value: object) -> bool | Domain: |
There was a problem hiding this comment.
Wait a second, do we really need to add the overload for object? It doesn't make sense because it overlaps with everything. I think the correct type hint is:
| @overload | |
| def __eq__(self, value: Dimension) -> bool: ... | |
| @overload | |
| def __eq__(self, value: core_defs.IntegralScalar) -> Domain: ... # type: ignore[overload-overlap] # intentionally returns Domain, not bool | |
| @overload | |
| def __eq__(self, value: object) -> bool: ... | |
| def __eq__(self, value: object) -> bool | Domain: | |
| @overload | |
| def __eq__(self, value: Dimension) -> bool: ... | |
| @overload | |
| def __eq__(self, value: core_defs.IntegralScalar) -> Domain: ... # type: ignore[overload-overlap] # intentionally returns Domain, not bool | |
| def __eq__(self, value: Dimension | core_defs.IntegralScalar) -> bool | Domain: |
There was a problem hiding this comment.
I think we want/need to support Dimension(...) == some_other_object comparison?
There was a problem hiding this comment.
If we go with your proposal, I should remove the else branch, right?
There was a problem hiding this comment.
But does it makes sense to support Dimension == Any? Shouldn't that just raise a TypeError? Something like:
| @overload | |
| def __eq__(self, value: Dimension) -> bool: ... | |
| @overload | |
| def __eq__(self, value: core_defs.IntegralScalar) -> Domain: ... # type: ignore[overload-overlap] # intentionally returns Domain, not bool | |
| @overload | |
| def __eq__(self, value: object) -> bool: ... | |
| def __eq__(self, value: object) -> bool | Domain: | |
| if isinstance(value, Dimension): | |
| return self.value == value.value | |
| if isinstance(value, core_defs.INTEGRAL_TYPES): | |
| int_value = cast(core_defs.IntegralScalar, value) | |
| return Domain(dims=(self,), ranges=(UnitRange(int_value, int_value + 1),)) | |
| raise TypeError(....) |
There was a problem hiding this comment.
I think we have cases, but I can try...
src/gt4py/next/common.py
Outdated
| @overload | ||
| def __ne__(self, value: Dimension) -> bool: ... | ||
| @overload | ||
| def __ne__(self, value: object) -> bool: ... | ||
| def __ne__(self, value: object) -> bool: |
| - `Dimension.__ne__(value)` raises `NotImplementedError` when called with an integer value, since it would produce two disjoint domains. | ||
| - `Domain.__or__` raises `NotImplementedError` for both multidimensional domains and for 1D domains that are disjoint (non-overlapping and non-adjacent). | ||
|
|
||
| The domain expression API only supports operations that result in a single contiguous `Domain`. |
There was a problem hiding this comment.
This is the only place in the ADR where contiguous is used. Everywhere else non-contiguous is used, is that on purpose?
There was a problem hiding this comment.
Ah forgot to push the file, because my directory wasn't clean and my brain filtered .md files when adding stuff to commit...
2 participants
Adds dimension comparison creating domains and basic domain union opertations as a first step towards
concat_where(I == 0, ..., ...).Limitations are described in the ADR.