Repository navigation
Conversation
Since #771 the `mapping` of an `NDRange` decides how blocked indices map to `ndrange` indices, and every backend derives the index, validity and linear index of a work item from `expand`, `in` and `linear_index`. That makes it possible for a package to launch kernels over an iteration space of its own, such as a list of indices, by specializing `partition` and `cartesian` for its launch object and `expand` for its mapping, but nothing said so, and a backend overriding `__validindex` for a generic context would silently break such an extension. The `NDRange` docstring now lists the functions a custom mapping defines, `partition` and the new `cartesian` and `expand` docstrings point to it, and the notes for backend implementations state that a backend must go through `expand`, `in` and `linear_index` and must not assume the `ndrange` of a context is a `CartesianIndices`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VHciC8x39gm97sABrSvBkt
Contributor
Benchmark ResultsShow table
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #781 +/- ##
=======================================
Coverage 64.89% 64.89%
=======================================
Files 23 23
Lines 2011 2011
=======================================
Hits 1305 1305
Misses 706 706 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vchuravy
reviewed
Sep 14, 2026
|
|
||
| A backend must not assume that `__ndrange(ctx)` is a `CartesianIndices` or that | ||
| `expand` is an affine map: the `mapping` field of the `NDRange` lets a package | ||
| define its own iteration space, for example a list of indices to visit, by |
Member
There was a problem hiding this comment.
This is a new contract, I am not supper happy about. These are private implementation details and a packing extending them is doing fishy work, and potentially type piracy.
I would be much happier if we had a public interface that folks could extend
abstract type Mapping end
And then Oceananigans could do its subtype of that, and so ndrange=Mapping()
giordano
commented
Oct 6, 2026
Comment on lines
+143
to
+160
| # Custom mappings | ||
|
|
||
| A package can iterate over a space of its own, for example a list of indices, by launching a | ||
| kernel with an `ndrange` object of its own type and defining: | ||
|
|
||
| - [`partition(kernel, ndrange, workgroupsize)`](@ref KernelAbstractions.partition) for that | ||
| type, returning an `NDRange` whose `mapping` describes the space, and whether the last | ||
| workgroup needs bounds-checking; | ||
| - [`cartesian(ndrange)`](@ref KernelAbstractions.cartesian) for that type, returning the | ||
| object stored as `ndrange` of the kernel context, which supports `Base.in` for a | ||
| `CartesianIndex` and [`linear_index`](@ref); | ||
| - [`expand`](@ref) for an `NDRange` with that mapping and `groupidx`, `idx` given as | ||
| `Integer` or `CartesianIndex`, returning the index handled by a work item, or an index that | ||
| is not `in` the `ndrange` object for a work item without one. | ||
|
|
||
| Backends check the validity of a work item as `expand(iterspace, groupidx, idx) in ndrange`, | ||
| so nothing else is needed for the kernel to see the mapped index through `@index`. | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
Is it ok to document this as part of the public contract? At this point, possibility to extend these three functions and then #848 is what we need in Oceananigans. I don't need to get this PR merged immediately (I still need to remove the extra stuff in docs/src/implementations.md), I just want to settle whether it's ok to extend these methods.
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
2 participants
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.
No description provided.