feat[next]: concat_where embedded (limited to 1D)#2545
feat[next]: concat_where embedded (limited to 1D)#2545havogt wants to merge 9 commits intoGridTools:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an embedded backend implementation of concat_where based on 1D domain regions (rather than a boolean mask field), along with updated tests and new pytest markers to track remaining unsupported cases (infinite domains, non-contiguous unions).
Changes:
- Implement
concat_wherefor embeddedNdArrayFieldusing 1D domain slicing/concatenation helpers (_invert_domain,_concat,_size0_field). - Update unit/integration tests to use domain conditions and add coverage for domain inversion and concatenation behavior.
- Introduce new pytest markers + skip/xfail wiring for embedded limitations (infinite domains, non-contiguous domain unions).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/next_tests/unit_tests/embedded_tests/test_nd_array_field.py | Updates concat_where unit tests to pass a Domain condition; adds tests for _invert_domain and _concat. |
| tests/next_tests/integration_tests/feature_tests/ffront_tests/test_concat_where.py | Adds embedded-specific markers for known unsupported cases; rewrites a multi-dim “lap-like” example using nested 1D concat_where. |
| tests/next_tests/definitions.py | Adds new marker IDs and updates embedded skip lists to stop blanket-xfailing uses_concat_where. |
| src/gt4py/next/ffront/experimental.py | Updates concat_where signature/docs to reflect domain-based condition semantics. |
| src/gt4py/next/embedded/nd_array_field.py | Implements embedded concat_where via domain inversion + concatenation; updates _concat to sort inputs. |
| pyproject.toml | Registers new pytest markers for embedded concat_where limitations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
egparedes
left a comment
There was a problem hiding this comment.
I have a couple of questions.
| if curr.domain[dim].unit_range.start < prev.domain[dim].unit_range.stop: | ||
| raise ValueError("Fields to concatenate must not overlap.") |
There was a problem hiding this comment.
I don't fully understand where the checks for the correctness of the input domains should be done. I mean, most of the private helper functions are only called from one place, so they could assume that the input domains have been already validated at the public entry point and skip other checks. However, the overlap check is done here while the contiguous check is done later when the result of another call returns None. I'm not sure if I'm missing something but, if possible, I'd suggest to concentrate the validation of the domains in a single point.
For example, one possibility
| if curr.domain[dim].unit_range.start < prev.domain[dim].unit_range.stop: | |
| raise ValueError("Fields to concatenate must not overlap.") | |
| if (left := prev.domain[dim].unit_range.stop) != (right := curr.domain[dim].unit_range.start): | |
| if left > right: | |
| raise ValueError("Fields to concatenate must not overlap.") | |
| if left < right: | |
| raise NotImplementedError("Fields to concatenate must be contiguous.") |
| # empty domain [3, 3) → normalized to [0, 0), invert sees (0, 0) | ||
| ( | ||
| common.Domain(dims=(D0,), ranges=(UnitRange(3, 3),)), | ||
| ( | ||
| common.Domain(dims=(D0,), ranges=(UnitRange(common.Infinity.NEGATIVE, 0),)), | ||
| common.Domain(dims=(D0,), ranges=(UnitRange(0, common.Infinity.POSITIVE),)), | ||
| ), |
There was a problem hiding this comment.
Question: wouldn't it make more sense to have a single (-inf, +inf) domain as inverted version of an empty domain?
No description provided.