Repository navigation
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mloubout
force-pushed
the
subgrid-strided-subsampling
branch
from
October 20, 2026 16:43
5489dc4 to
0102bc4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3080 +/- ##
==========================================
+ Coverage 84.16% 84.27% +0.10%
==========================================
Files 258 260 +2
Lines 57290 57945 +655
Branches 4893 4922 +29
==========================================
+ Hits 48220 48832 +612
- Misses 8244 8287 +43
Partials 826 826
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mloubout
added a commit
that referenced
this pull request
Oct 20, 2026
Address review on #3080. Rather than striding the parent's loop by `factor`, iterate the subsampled Dimension itself with a unit increment and rescale parent accesses: for (x = x_m; x <= x_M; x += 1) for (xs = xs_m; xs <= xs_M; xs += 1) if (x%xf == 0) -> g[xs] = u[xs*xf] g[x/xf] = u[x] A non-unitary increment escapes the Lamport-style dependence analysis, so it is avoided. The loop bounds are the ConditionalDimension's own runtime arguments, which removes the need to anchor the loop minimum, and with it the `Strided` property, the `NodeIteration.limits` hook and the hand-rolled round-up helper. The pass moves into `_lower_clusters` so every pipeline picks it up rather than just the CPU and GPU `advanced` ones; it now applies under `noop` too. Also fix `ConditionalDimension._arg_values`, which derived the subsampled max with `ceil` rather than `floor`. With `factor=3` and `x_M=11` that gave `xs_M=4` while the last subsampled point is `x=9`, i.e. index 3 -- harmless while a guard filtered the extra point, but a one-past-the-end write once the bound drives the loop directly.
FabioLuporini
approved these changes
Oct 20, 2026
| if (x%xf == 0) -> g[x/xf] = ... | ||
| g[x/xf] = ... | ||
|
|
||
| The loop is taken over the subsampled Dimension `cd` itself, with a *unit* |
| # The first subsampled point at or after the parent's min... | ||
| with suppress(KeyError, TypeError): | ||
| vals[self.min_name] = toint(kwargs.get(self.parent.min_name)) | ||
| vals[self.min_name] = math.ceil(kwargs.get(self.parent.min_name) / fact) |
Contributor
There was a problem hiding this comment.
to be sure, '/' or '//' ?
Contributor
Author
There was a problem hiding this comment.
/, // would be floorinstead of ceil.
A Function subsampled by `factor` along `d` was written under a `d % factor == 0` guard, so the enclosing loop visited every point of `d` and discarded all but one in `factor` of them. Where legal, drop the guard and step the loop `factor` points at a time instead. The rewrite is recorded as a parameterized `Strided(factor)` property on `d`; `NodeIteration.limits` applies the stride and rounds the loop min up onto the subsampling grid. The iteration space stays keyed on `d`, so runtime bounds, domain decomposition and non-subsampled accesses are all unaffected. Clusters are grouped by iteration space prefix alone, not by guard: a guard may only become a stride if every Cluster sharing the prefix carries the same one, since dropping it is what makes a loop nest shared.
Codegen, numerics against numpy slicing, loop-min anchoring across offset bounds, and the cases that must keep a pointwise guard: a loop nest shared with an unguarded equation, a sequential Dimension, an extra `condition`, and non-affine accesses.
Walks through declaring a subsampled Function and the code generated for it: striding the outer Dimension, the innermost one, and several at once, the anchoring of the loop min, and the four cases that fall back to a pointwise guard. Every case prints its generated C.
A coarser Grid whose points are a subset of a parent Grid's points: along
each Dimension subsampled by `factor`, point `i` sits at parent index
`i*factor`.
sub = SubGrid(grid, factor=2)
f = Function(name='f', grid=sub)
Dimensions, shape and spacing are all derived, so a Function on a SubGrid
needs neither `shape` nor `dimensions`; previously both had to be worked out
by hand. The Dimensions are ConditionalDimensions, so the strided lowering
applies unchanged, and a factor of 1 reuses the parent Dimension untouched.
The parent's stepping Dimension is shared rather than derived afresh: a
SubGrid coarsens space only, so a TimeFunction on it must step over the same
buffer indices as one on the parent.
Non-integer factors are rejected for now -- they place SubGrid points between
parent points, which requires interpolation.
Address review on #3080. Rather than striding the parent's loop by `factor`, iterate the subsampled Dimension itself with a unit increment and rescale parent accesses: for (x = x_m; x <= x_M; x += 1) for (xs = xs_m; xs <= xs_M; xs += 1) if (x%xf == 0) -> g[xs] = u[xs*xf] g[x/xf] = u[x] A non-unitary increment escapes the Lamport-style dependence analysis, so it is avoided. The loop bounds are the ConditionalDimension's own runtime arguments, which removes the need to anchor the loop minimum, and with it the `Strided` property, the `NodeIteration.limits` hook and the hand-rolled round-up helper. The pass moves into `_lower_clusters` so every pipeline picks it up rather than just the CPU and GPU `advanced` ones; it now applies under `noop` too. Also fix `ConditionalDimension._arg_values`, which derived the subsampled max with `ceil` rather than `floor`. With `factor=3` and `x_M=11` that gave `xs_M=4` while the last subsampled point is `x=9`, i.e. index 3 -- harmless while a guard filtered the extra point, but a one-past-the-end write once the bound drives the loop directly.
`IterationSpace.switch` remaps intervals, sub-iterators and directions, but not properties, so swapping the parent Dimension for the subsampled one left the latter with none: the loop lost PARALLEL and VECTORIZED, and with them every OpenMP pragma. Transfer them explicitly, as `blocking` does for its BlockDimensions. The subsampled Dimension covers the same points as its parent, just addressed in subsampled coordinates, so it inherits the parent's properties unchanged. Also broaden the tests and tutorial to exercise SubGrid end to end: the shared-nest, sequential and non-affine cases now build their Functions from a SubGrid and run the Operator with checked results, rather than only inspecting the generated code.
The tutorial was organised around the compiler -- "the generated loop", "when a guard is used instead" -- with SubGrid relegated to a setup step, so the API it is meant to document was never actually covered. Lead with the API instead: creating a SubGrid and what it derives from the parent, what it rejects and why, defining Functions and TimeFunctions on it, writing Operators in both directions, finite differences over the coarse spacing, and the two things SubGrid deliberately leaves to a bare ConditionalDimension (time subsampling and extra conditions). The codegen material follows as a closing section for readers profiling the generated C. Every Operator in the tutorial now runs, with the result asserted.
Both said what was disallowed without saying what to do instead.
Nesting now names the equivalent single-level call, since the composed factor
is just the product of the two:
Cannot subsample a SubGrid. `3` applied to a SubGrid of factor `(2, 2)` is
the same as factor `(6, 6)` applied to its parent Grid directly, so use
`SubGrid(parent.parent, factor=(6, 6))`
This needs the factors validated first, so the check moves after
`__factors_setup__`.
A non-integer factor now names the nearest legal values, clamped to >= 1 so
the suggestion is never itself rejected.
Tests assert the suggested factors are both legal and equivalent to the
two-step subsampling they replace, so the messages cannot drift.
Two CI failures on this branch, both in the new material rather than the compiler. `test_parent_access_rescaled` asserted on literal substrings of the emitted C (`g[xs]`), which only hold when linearization is off; the CXX jobs render the same access as the macro `gL0(xs, y)`. Assert on the lowered Expression instead, selecting the one writing `g` since linearization adds Size/Stride Expressions alongside it. Verified against all four CI language settings: C, CXX, openmp, CXXopenmp. The tutorial's stored outputs were stale: the error-message cells still showed the wording from before the previous commit reworded them, so nbval compared them against the new text and failed. Re-executed.
Fix `SubGrid.root` to return the parent's root. Like a SubDomain, a SubGrid is a view of its parent, and `Operator._prepare_arguments` collapses the discretizations it finds by `root` to tell several views of one Grid apart from several unrelated Grids -- rejecting the latter under MPI. Without this, any Operator touching both a Grid and a SubGrid of it raised "Multiple Grids found", so subsampling could not be used with MPI at all. With that fixed, add the tests: a strided copy against numpy slicing for factors 2/3/4, the codegen still being a unit-stride subgrid loop, the per-rank bounds tiling the global subgrid with no gaps or overlap, the SubGrid sharing its parent's decomposition, and time subsampling. All on 2 and 4 ranks. Note a reduction into a subsampled Function (`Inc(g, u)`) is rejected under MPI with "Unsupported MPI for the given equations", so the partitioning check uses a sentinel plus an allreduce over the local sizes rather than Inc.
`Inc` into a factor-subsampled Function was classified PARALLEL_IF_ATOMIC, so an OpenMP array reduction was emitted over the whole Function. With nine such Functions the thread-private copies came to ~13 MiB and the kernel segfaulted; four arrays worked, six did not, which is the size of the default stack. The dependence is reported with an all-zero distance vector, so the write is plainly independent across iterations. What pushed it into the reduction branch was `is_storage_related`, which treats any NonlinearDerived Dimension as capable of mapping several iterations onto one location. That is true of a SteppingDimension, where `time % 2` deliberately revisits a slot, but not of a factor-based ConditionalDimension: the guard admits only `parent % factor == 0`, and `i -> i//factor` is one-to-one on those. Add `Dimension.is_Injective`, true exactly for that case, and exempt it. An arbitrary `condition`, or a factor combined with a condition, carries no such promise and stays non-injective.
mloubout
force-pushed
the
subgrid-strided-subsampling
branch
from
October 21, 2026 18:43
a25a088 to
c5e56cd
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support strided subgrids.