-
Notifications
You must be signed in to change notification settings - Fork 55
feat[next]: Embedded domain construction from dimension comparison #2532
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
Changes from 3 commits
09dd736
5ec9bd3
d62e51a
9598eee
7f6393d
7205478
991d93a
99a9935
08eaec3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| --- | ||
| tags: [] | ||
| --- | ||
|
|
||
| # Limitations of embedded concat_where | ||
|
|
||
| - **Status**: valid | ||
| - **Authors**: Hannes Vogt (@havogt) | ||
| - **Created**: 2026-03-12 | ||
| - **Updated**: 2026-03-17 | ||
|
|
||
| In embedded execution, `concat_where` is, for now, limited to simple but common cases. | ||
|
|
||
| We do not support `concat_where` in cases | ||
|
|
||
| - where the domain would be infinite and therefore can't be represented as an ndarray, e.g. `concat_where(I < 0, 0.0, somefield)` where the scalar 0.0 would be broadcasted to a field reaching to -infinity; | ||
| - with multi-dimensional domains, e.g. `concat_where(I > 0 | J > 0, a, b)`. These cases need to be represented by a nested `concat_where(I > 0, a, concat_where(J > 0, a, b))`; | ||
| - with non-consecutive (disjoint) domain conditions, e.g. `concat_where(I != 0, a, b)`. These cases need to be expressed using nested `concat_where`, e.g. `concat_where(I < 0, a, concat_where(I > 0, a, b))`. | ||
|
|
||
| ## Context | ||
|
|
||
| `concat_where` requires expressing conditions like `I != i`, which would produce two disjoint 1D domains (everything before index `i` and everything after). We need a way to represent these non-consecutive domains. | ||
|
|
||
| A complete implementation would require designing how to handle fields on non-hypercubic domains. Currently, `Domain` is a Cartesian product of per-dimension `UnitRange`s, which inherently describes hypercubic (rectangular) regions. Supporting arbitrary non-consecutive domains in multiple dimensions would mean fields could live on non-rectangular regions, requiring fundamental changes to field storage, slicing, and iteration. | ||
|
|
||
| ## Decision | ||
|
|
||
| Non-consecutive (disjoint) domains are **not supported** in the domain expression API: | ||
|
|
||
| - `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`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only place in the ADR where
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah forgot to push the file, because my directory wasn't clean and my brain filtered .md files when adding stuff to commit... |
||
|
|
||
| ## Consequences | ||
|
|
||
| - `concat_where` with `I != i` must be rewritten as `concat_where(I < i, ..., concat_where(I > i, ..., ...))`. | ||
| - This keeps the domain expression API simple: all supported operations return a single `Domain`. | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| ### General `concat_where` with multi-dimensional domain conditions | ||
|
|
||
| Implementation for multi-dimensional domain conditions (e.g. `(I != 2) | (K != 5)`) and full support for domain operations in `concat_where` would require | ||
|
|
||
| 1. **A `DomainTuple` class** with full algebra: a `tuple` subclass carrying `__and__`, `__or__`, `__rand__`, `__ror__` operators so that expressions like `tuple & Domain`, `Domain & tuple`, and `tuple | tuple` all work. | ||
|
|
||
| 2. **Normalization of domain tuples**: We need to design `DomainTuple` invariants, e.g. | ||
|
|
||
| - Should all domains be promoted to the same rank (missing dimensions filled with infinite ranges)? | ||
| - Should we reduce overlapping domains to non-overlapping via box subtraction? | ||
|
|
||
| Before implementing a complex `DomainTuple`, we should conclude on (if we want) a concept of non-consecutive fields. | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -108,6 +108,49 @@ def __add__(self, offset: int) -> Connectivity: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __sub__(self, offset: int) -> Connectivity: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return self + (-offset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __gt__(self, value: core_defs.IntegralScalar) -> Domain: # type: ignore[misc] # returns Domain, not bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Domain(dims=(self,), ranges=(UnitRange(value + 1, Infinity.POSITIVE),)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __ge__(self, value: core_defs.IntegralScalar) -> Domain: # type: ignore[misc] # returns Domain, not bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Domain(dims=(self,), ranges=(UnitRange(value, Infinity.POSITIVE),)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __lt__(self, value: core_defs.IntegralScalar) -> Domain: # type: ignore[misc] # returns Domain, not bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Domain(dims=(self,), ranges=(UnitRange(Infinity.NEGATIVE, value),)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __le__(self, value: core_defs.IntegralScalar) -> Domain: # type: ignore[misc] # returns Domain, not bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Domain(dims=(self,), ranges=(UnitRange(Infinity.NEGATIVE, value + 1),)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @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: ... | |
| def __eq__(self, value: object) -> Literal[False]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy doesn't like it: Overloaded function signatures 1 and 3 overlap with incompatible return types
havogt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want/need to support Dimension(...) == some_other_object comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with your proposal, I should remove the else branch, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have cases, but I can try...
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
Uh oh!
There was an error while loading. Please reload this page.