Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 3 medium |
🟢 Metrics 167 complexity · 20 duplication
Metric Results Complexity 167 Duplication 20
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This was referenced Jul 27, 2026
gouarin
force-pushed
the
bc-prediction-position
branch
from
July 27, 2026 14:46
ed19790 to
ff57f1b
Compare
gouarin
force-pushed
the
bc-prediction-position
branch
from
September 2, 2026 08:09
6b5062a to
7ff8f58
Compare
1 task
gouarin
added a commit
that referenced
this pull request
Sep 2, 2026
## Description `make_graduation_width_test_tag`, added in #498, built the mesh as a local variable and returned a tag on it. A field only points to its mesh (`inner_mesh_type` stores a raw pointer), so `graduation.width_zero_is_a_noop` and `graduation.width_above_dispatch_range_throws` run `samurai::graduation` on a destroyed mesh. On `main` the two tests pass by luck: the stack slot is still intact when the tag is read. On the branches rebased on `main` today (#492, #493, #494) the full test run fails with ``` Expected: samurai::graduation(tag, stencil) doesn't throw an exception. Actual: it throws std::out_of_range with description "LevelCellArray::get_interval: interval not found at level 4098, i = [0,8[@0:2, index = ". ``` while the same test passes when run alone. AddressSanitizer confirms the cause on `main` itself: ``` ERROR: AddressSanitizer: stack-use-after-return #0 samurai::graduation<...> include/samurai/algorithm/graduation.hpp:141 #1 graduation_width_zero_is_a_noop_Test::TestBody() tests/test_graduation.cpp:284 Address is located in stack of thread T0 at offset ... in frame #0 make_graduation_width_test_tag tests/test_graduation.cpp:267 ``` The tag is now built on `holder(mesh)`: the field owns a copy of the mesh, so the helper is safe by construction, the way `reconstruction()` already returns a field that outlives its local mesh. Returning the mesh and building the tag in each test was considered and rejected: it keeps the lifetime rule at every call site, and a one-liner passing a temporary mesh would recreate the bug without any warning. ## How has this been tested? - `tests/test_graduation.cpp`, built with `-fsanitize=address`: the report above before the change, clean after it, `graduation.*` all passing. - `pre-commit run --files tests/test_graduation.cpp`. ## Code of Conduct By submitting this PR, you agree to follow our [Code of Conduct](https://github.com/hpc-maths/samurai/blob/master/docs/CODE_OF_CONDUCT.md) - [x] I agree to follow this project's Code of Conduct
numeric/prediction_coefficients.hpp answers what the coefficients are once the shift is known; nothing yet answers what the shift is at a given cell. This adds that query. No consumer uses it, so nothing changes behaviour. for_each_prediction_shift_run() walks one interval and hands back the maximal runs over which the shift is constant, one shift per direction. Three properties it is built around: - it classifies against the global, replicated domain(level), never against the cells one rank happens to hold, which is what makes the answer partition independent with no communication. Whether those cells are present locally is a separate halo question; - it is one query per interval rather than per cell, and the bulk of an interval comes back as a single run with every shift zero, so a consumer keeps its hoisted kernel there and cannot move an interior value; - it is exact on a holed domain. An interval passing over the edge of a hole is split, because classifying the whole interval by its worst cell would shift cells that need no shift. A periodic direction has no boundary: stepping off the end of one reaches the cells the periodic exchange fills from, so nothing is clamped there. The caller passes the wrap per direction - the same quantity update_ghost_periodic shifts by - and 0 where the direction is not periodic. A hole still clamps in a periodic direction, because only stepping off the end of the domain wraps. Tested without a mesh, the answer being a property of the domain alone: the shift at every distance from a boundary in 1D, 2D and 3D at radius 1 and 2; the decomposition of an interval crossing a hole, cell for cell; periodicity per direction; and a cross-check against a slow per-cell implementation over a deliberately awkward domain - two holes, one of them one cell wide so that no stencil fits across it, one biting into the edge, and a block hanging off the side - with a guard asserting the comparison actually meets every shift a radius-1 stencil can take, a cell outside the domain, and a cell that does not fit.
The consumers apply the 1D coefficient family as a tensor product, so the cells a shifted stencil reads are a whole box, mixed terms included. Availability read one direction at a time cannot see a cell that is missing only diagonally: at the cell diagonally off the corner of a hole every direction reads cells the domain has, yet the corner of the box is inside the hole, and in the new design nothing fills it. The query now asks the domain about the box. A shift is admissible when the domain holds all of it, and the answer is the most centred admissible one: least shifted overall, then shifting x least (a transverse shift only picks a different row, an x shift moves the innermost loop's reads), then negative, so that the answer never depends on how the domain is stored. On a box domain the two rules agree at every cell, corners included, so this is a statement about re-entrant corners only, whether they belong to a hole or to an L-shaped domain. It also makes fits the joint condition, which is strictly stronger: a plus-shaped domain is three cells wide in each direction and still holds no 3x3 box. Neighbouring runs carrying the same shift are merged, so the runs are maximal in the strict sense the docstring claims and a consumer launches one kernel per genuine change of shift. The slow reference the tests cross-check against was rewritten to the box rule, and it is independent of the machinery: reversing the tie-break in the query alone fails five tests. The 3D sweep now also runs periodic in all three directions, the only case where two transverse directions step off the end of the domain at once.
Same answers, same tests. The query used to be one loop interleaving the row cursors, the availability counts, the periodic top-up, the breakpoint computation and the shift search; it is now named stages: - DomainRow::around() returns how far one row of the periodically extended domain covers around a cell, wrap along the row included, so the driver never mentions the period; - displaced_row() keeps the transverse half of the wrap and enumerates only the directions that can have stepped off the end; - most_centred_fit() picks the shift from the covers alone; - TransverseRows owns both the enumeration of the reachable rows and the index of an offset in it, so the shift table and the row array agree by construction rather than by convention. Small diagrams document the re-entrant corner, the reach and the run decomposition over a hole.
The black-box suite survived the refactor unchanged, which is what it is for; two properties live below what it can observe: - index_of inverts offset, checked at compile time: the shift table and the row array index rows the same way by construction, and a change of enumeration order on one side alone stops compiling; - DomainRow::around reports exactly where its answer stops holding. A cover cut too early only fragments the sweep before the merge glues the pieces back together, so the one-query-per-interval cost rests on a property the public run decomposition cannot show - including that a periodic wrap tops the counts up without moving the breakpoints.
gouarin
force-pushed
the
bc-prediction-position
branch
2 times, most recently
from
September 2, 2026 15:24
63a8240 to
2f246e8
Compare
The box-rule search tabulated, for every candidate shift, the indices of the rows its stencil box covers: `(2r+1)^dim x (2r+1)^(dim-1)` entries, which is 16 GB of static storage at radius 3 in six dimensions - the instantiation the prediction roundtrip test makes as soon as a consumer uses the query, and the binary then fails to link. The candidates stay tabulated, on the heap; the rows of a candidate are a few integer operations and are recomputed where they are needed. The per-query arrays of row cursors and row covers had the same shape, `(4r+1)^(dim-1)` entries: five in 2D at radius 1, 371293 in the same six dimensions, which is not a stack object either. They now live on the stack while small and on the heap otherwise. TransverseRows is parameterised by its reach rather than by the radius it is `2r` of, which is what it always was and what a later consumer will need at another reach.
gouarin
force-pushed
the
bc-prediction-position
branch
from
September 2, 2026 15:30
2f246e8 to
26b0523
Compare
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
1 participant
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.
Description
prediction_shifts.hpp: given a cell, what shift must the prediction stencil take so that it readsonly cells the domain has. #490 answered the other half - given a shift, what are the coefficients -
and is deliberately mesh-free; this is the half that looks at a domain.
No consumer calls it, so nothing changes behaviour. It is the groundwork for the boundary
rewrite, as #490 was.
for_each_prediction_shift_run<radius>(domain, period, i, index, f)walks one interval and returnsthe maximal runs of constant shift, one shift per direction. Four properties to expect:
domain(level), never against one rank's cells: theanswer is the same on every rank with no communication;
with every shift zero - a consumer keeps its hoisted kernel there;
cells that must be shifted next to cells that must not;
family as a tensor product, so at the cell diagonally off a re-entrant corner every direction on
its own looks fine while the box reads the hole's corner. Among the admissible shifts the query
takes the most centred one, with a fixed tie-break so the answer cannot depend on how the domain
is stored - and
fitsis accordingly a joint condition, strictly stronger than every directionbeing wide enough on its own.
A periodic direction has no boundary, so nothing is clamped there. The caller passes the wrap per
direction - the same quantity
update_ghost_periodicshifts by - and0where the direction is notperiodic. The parameter is required rather than defaulted, because silently treating a periodic
direction as bounded would move values for nothing.
The query is written in named stages - the cover of one row of the periodically extended domain
(wrap along the row included), the transverse half of the wrap, the shift search over tabulated
candidates - each documented in the header with small diagrams. The contract between the shift
table and the row array is owned by one type (
TransverseRowsholds both the enumeration and theindexing) rather than by a convention between two functions.
The tables and buffers of the query, off the stack (last commit)
The box-rule search tabulated, for every candidate shift, the indices of the rows its stencil
box covers:
(2r+1)^dim x (2r+1)^(dim-1)entries, 16 GB of static storage at radius 3 in sixdimensions, which is what the prediction roundtrip test instantiates as soon as a consumer uses
the query - the test binary then fails to link. The candidates stay tabulated, on the heap, and
the rows of a candidate are recomputed where they are needed. The per-query arrays of row
cursors and covers,
(4r+1)^(dim-1)entries each, live on the stack while small and on the heapotherwise.
How has this been tested?
tests/test_prediction_shifts.cpp, 22 tests, all mesh-free. Beyond the expected shifts in 1D/2D/3Dat radius 1 and 2, three carry the weight:
domain (two holes, one of them one cell wide so no stencil fits across it, one biting into the
edge, plus a block hanging off the side), with a guard that the comparison really meets every
shift, a cell outside the domain and a cell where nothing fits;
domain - the partition-independence invariant, written so that it says what going wrong looks
like;
same way (a compile-time check), and one row's cover reports exactly where it stops holding - the
property the one-query-per-interval cost rests on, invisible from the public decomposition
because a cover cut too early only fragments the sweep before equal runs are merged back.
Code of Conduct