Bring master up to the xmsconan 2.16 build and the time-stepped tracer - #11
Merged
Conversation
Replace the Conan 1 recipe, hand-written CMakeLists.txt, build.py and the Travis/AppVeyor pipelines with a build.toml consumed by `xmsconan gen` and `xmsconan ci`. - build.toml declares the sources, pybind bindings and dependencies. conanfile.py, build.py, CMakeLists.txt, pytest.ini, .flake8, xms_conan2_file.py and _package/pyproject.toml are now generated and gitignored. The ignore patterns are anchored to the repo root so the hand-maintained test_package/ files are not swept up. - CI is now .github/workflows/XmsGridtrace-CI.yaml (flake, mac, linux, windows), with the Windows matrix fanning out over Python 3.10/3.13. - Dependencies pinned to current releases, matching the set that xmsextractor 10.0.6 itself pins: xmscore 7.0.8, xmsgrid 9.0.9, xmsinterp 7.0.8, xmsextractor 10.0.6. - Python bindings move to the wheel layout the generated recipe installs into: the pybind module is renamed xmsgridtrace -> _xmsgridtrace, and a pure-Python wrapper lives in _package/xms/gridtrace/. The public import path is now `from xms.gridtrace import GridTrace`. - Python tests move to _package/tests/ and use the xms.grid UGrid API. - test_package/ ported from the Conan 1 API to Conan 2. - pydocs and the README badge updated for the new module path and CI.
The previous pins were xmsgrid 2.x / xmsinterp 2.x. Moving to xmsgrid 9.0.9 and xmsextractor 10.0.6 brings two breaking upstream changes that broke the Linux and Windows builds: - geoms.h moved from xmsinterp to xmsgrid. Repoint the include to <xmsgrid/geometry/geoms.h>. (No gm* symbol is actually referenced by XmGridTrace.cpp, so this include is a candidate for removal later; repointing keeps the change minimal and preserves transitive includes.) - XmUGrid is now passed as std::shared_ptr, not BSHP/boost::shared_ptr. XmUGrid::New returns std::shared_ptr, and the extractor factories take std::shared_ptr. Convert every XmUGrid handle in xmsgridtrace to match, including the pybind init. This mirrors xmsextractor 10.0.6, which keeps BSHP for its own objects but takes std::shared_ptr<XmUGrid>. Verified statically against the pinned dependency tags: all 17 xms includes resolve, the three New factories match their signatures, and every extractor method called still exists.
xmscore 7.0.8 -> 7.0.11, xmsgrid 9.0.9 -> 9.0.10, xmsinterp 7.0.8 -> 7.0.9, xmsextractor 10.0.6 -> 10.0.7. All four are now built under conan~=2.31.0 (so their binaries resolve) and with xmsconan 2.16.0 (so their libraries carry the testing helpers this repo links).
The "follow flow path" vector display option is being routed through XmGridTrace so the traced path integrates a time-varying field. That only works if the tracer is fast enough to run for every visible glyph, so this establishes what it costs today. testTraceBenchmark traces N seeds over a 200x200 quad grid loaded with two timesteps of a vortex field whose rotation reverses between them, across three seed populations: interior far enough from the edge that no trace can reach it -- pure stepping cost boundary in a band along the perimeter -- forces the out-of-domain exit branch mixed spread over the whole domain -- what the display actually does Separating them matters: the populations turn out to differ by two orders of magnitude per seed, and an undifferentiated average would have hidden that. Alongside wall time it reports an ExtractData call count, from a CXX_TEST-only counter incremented where the four per-step searches happen. Without it an optimization cannot be shown to have removed searches rather than merely found a faster machine. It also reports a setup breakdown -- BuildTriangles, the GmTriSearch R-tree build, and an activity-only reapply timed separately -- because which of those dominates decides whether triangulations can be shared across timesteps. XMGT_BENCH_SEEDS and XMGT_BENCH_CELLS size the run so a sweep needs no recompile. The defaults are small enough to leave the case in the regular suite, and the assertions are order-of-magnitude guards rather than tight bounds so it will not go flaky on a shared runner. One assertion is deliberately loose for a measured reason: a seed that exits the grid on its first step can reach the points.size() < 3 early return and come back with only the seed point, so "every seed yields a usable polyline" is false. It shows up at roughly 1 in 100,000.
…er exit
TracePoint constructed a fresh XmUGrid2dPolylineDataExtractor on every out-of-domain
step, inside the stepping loop. That constructor triangulates the whole grid, and the
SetPolyline that follows indexes every triangle into a new GmMultiPolyIntersector.
Both depend only on the grid, which cannot change during a trace, and both were thrown
away at the end of the if block and rebuilt for the next exiting particle.
Benchmarked on a 200x200 grid, one exit event cost ~40 ms against ~48 us for a
complete interior trace -- roughly 830x. On a seed population with a 5% exit rate,
those 5% accounted for 98% of total trace time.
It is now a member built lazily on the first exit and reused. Three things make the
reuse safe, each checked in xmsextractor rather than assumed:
- BuildTriangles is guarded by m_triangleType != a_location, so the second
SetPolyline skips the triangulation.
- ComputeExtractLocations builds m_multiPolyIntersector only when null and clears
its output locations at entry, so no state carries between polylines.
- XmGridTrace consumes only GetExtractLocations(), never extracted values, so the
dummy zero scalars the constructor installs are irrelevant and the instance stays
valid for the tracer's lifetime.
m_ugrid is fixed at construction with no setter, so a cached extractor cannot outlive
the grid it was built for. The member stays null until a trace actually exits, so a
seed population that never reaches a boundary pays no memory for it.
Measured A/B on one machine state at 1,000 seeds: the boundary population goes from
15,996 to 71 us/seed (224x) and the realistic mixed population from 2,200 to 51
us/seed (43x), while the interior population -- which never exits, and is the control
-- moves 1%. Per exit the cost falls from 40 ms to ~9.5 us. ExtractData counts are
identical before and after: this removes no searches at all, only whole-grid rebuilds,
so the separate search-reduction work is still available on top of it.
testBoundaryExtractorIsCached is the guard. Caching is invisible in the output, so the
assertion that matters is a construction count from a CXX_TEST-only counter; the test
also compares the two traces to 1e-12, which is what would catch reuse silently
changing an answer.
GetVectorAtLocationAndTime weighted each timestep by its own distance from the current
time, so perc1 was |t - t1| / totalTime and multiplied timestep 1's value. At t == t1
that weight is zero: a particle released exactly at the first timestep was advected
entirely by the field at the second. A timestep is weighted by its *closeness* to the
current time, so the distance from one timestep is the weight of the other.
This is the defect that makes the tracer worth using at all. Routing the "follow flow
path" display option through it is only an improvement if the trace follows the field
as it changes, and an inverted blend does not.
Propagating XM_NODATA is part of the same fix rather than a separate one. With the
weights corrected, a location that is inactive in only one of the two timesteps stops
resolving to the sentinel and starts resolving to a blend of it: 0.9 * 0.1 + 0.1 *
-9999999 is -999999.9, which is neither no-data nor a velocity, and which passes every
no-data test the callers make. The sentinel is now propagated when either bracketing
timestep has no data at the location, which is what makes "a cell active at t1 but
inactive at t2 terminates the trace" actually hold. testStartInactiveCell was passing
before only because the inverted weights happened to give timestep 2 all the weight at
t == t1; with the propagation it passes for the right reason.
testTimeVaryingFieldChangesPath is the regression guard. One cell spanning the domain
gives a spatially uniform field, so any curvature in the path can only have come from
time; the field rotates +x -> +y between timesteps rather than reversing, so the
interpolated velocity never passes through zero and cannot trip the velocity-is-zero
exit partway along. Its first assertion is the one that catches an inversion -- a
particle released at t1 must step due east with y untouched -- and it compares against
a frozen-field control traced by the same code, which never turns.
Three existing baselines move. Each first step was checked by hand rather than
accepted from the runner:
testUniqueTimeSteps 0.5 -> 0.6, the t1 cell value 0.1 over dt 1, where it was 0.7
from t2's 0.2. Its second step, 0.9*0.11 + 0.1*0.21 = 0.12,
matches the recorded 0.744 to the digit.
testInactiveCell same first step, and the trace now terminates exactly at x = 1,
the boundary of the cell that is inactive at t2, rather than at
0.9979 -- it had been stopping just short via a max-change-
velocity blow-up on a no-data-contaminated blend.
testTutorial first step y 0.5 -> 1.5: corner scalars interpolate to (0, 0.5),
times the multiplier of 2, over dt 1. The old 1.25 was a
boundary-clipping artifact of using the doubled second-timestep
field at t = 0 -- the tutorial's own comment says the second
timestep is doubled to show an increase, so the trace should
start at the first timestep's magnitude and speed up.
The "follow flow path" display traces every visible vector glyph, tens of thousands of them per redraw, driven from Python. One TracePoint call per glyph pays a language boundary crossing per glyph for work that is identical across them, and it can only report why the *last* trace ended -- GetExitMessage describes a single operation, so a caller has no way to ask why glyph 4,000 stopped short. TracePoints takes all the seeds at once and returns a polyline, a time array, and an exit message per seed. It does not advance the time steps: every trace runs against whichever pair AddGridScalarsAtTime most recently supplied, and a caller wanting traces that span more of a series feeds the next step and traces again. Keeping that in the caller is deliberate -- the two-step window is instance state, so a batch that advanced it internally would have to carry per-seed continuation state, which is a different and larger design than this one. Mismatched input lengths return nothing rather than tracing the common prefix. A caller that supplied the wrong number of start times has a bug, and a partial result lets it go unnoticed. TracePoint's two output arrays could also come back different lengths, which this fixes because the batch documents them as parallel. The position push was conditional on the step actually moving while the time push was unconditional, so a step shorter than XM_ZERO_TOL left the times array one longer and silently misaligned every later pair -- undetectable to a caller zipping them. The time is now pushed only when the point is. No existing expectation moves, so no recorded trace contained such a step. testTracePointsMatchesSerialTracePoint compares the batch against serial TracePoint calls on an identical fixture rather than against a recorded baseline, which would drift with the tracer instead of pinning the equivalence. Its seeds cover the three shapes a caller has to handle: two traces that leave the grid, and a seed outside it that yields an empty trace rather than a polyline.
…n enum
A trace can only run as far as the second of the two loaded time steps, because that is
as far as the field is known. The batch added in the previous commit therefore traced
every seed to the edge of one window and threw away everything it knew, which is not
tracing a flow through time -- it is tracing it through one interval. Restarting from
the last position would not fix that either, because a restart loses the trace's history.
Traces now suspend and resume. StartTraces seeds a batch, ContinueTraces advances every
unfinished trace and returns how many are waiting on a later time step, and the caller
feeds the next one and calls again:
tracer->StartTraces(seeds, seedTimes);
while (tracer->ContinueTraces() > 0 && series.HasNext())
tracer->AddGridScalarsAtTime(series.Next(), ...);
tracer->GetTraceResults(traces, times, reasons);
Keeping the two-step window means memory stays bounded however long the series is, and
the caller reads time steps only as the traces actually need them. Stopping early is
legitimate: traces still waiting end where they got to, and say so.
The substance is what survives a window change. Position and time are the obvious ones.
The distance and elapsed-time budgets are whole-trace, not per-window, so they carry.
So do the adaptive step size and the previous velocity, because the subdivision tests
compare each step against the one before it -- restarting those at a boundary would kink
the path exactly where the time step changes, which is the one place this has to be
smooth. TracePoint's body is now StepTrace, which either starts a trace or resumes one;
every exit from it routes through a single lambda that writes that state back, so there
is no path that advances a trace without recording where it reached.
Resumability cannot be read off the loop's final state, so it is tracked explicitly: a
subdivision puts the trace back in motion *after* the time step clamp has already fired,
and several conditions in one iteration overwrite each other.
This also only works because of the interpolation fix. A trace resuming at the new first
time step is advected by that step's field; under the inverted weights it would have used
the following one, so every window boundary would have introduced an error.
Since nothing outside this repository uses XmGridTrace, the surrounding API is cleaned up
rather than extended around:
- The exit reason is an enum, not a message. A caller has to tell "left the grid, draw
it short" from "spent its distance budget, this is the normal ending" for tens of
thousands of seeds, and string comparison cannot support that -- the old messages were
composed by appending, so no fixed string identified a case. The strings remain, one
per reason, for logs and tooltips. TracePoint gets GetExitReason so the single-point
path can answer the same question the batch answers.
- The one-shot TracePoints from the previous commit is gone; the resumable trio subsumes
it, and one way to batch is better than two.
- GetExitMessage returns const std::string& and is const.
- AddGridScalarsAtTime takes activity by const reference.
testTracesContinueAcrossTimeSteps is the guard, over a field rotating +x -> +y -> -x. Its
strongest assertion is not that the continued trace is longer: it is that the trace never
given the third time step is an exact prefix of the one that was. That is what shows
resuming extends the path rather than recomputing it, and it is what fails if any carried
state is dropped at the boundary. It also checks the path turns back on itself, which no
single pair of those time steps can produce.
Measured on the benchmark's realistic seed population, 43 of 250 seeds stop waiting for a
later time step -- traces the previous design silently truncated.
The Python bindings still compile against this by inspection: their three uses are lambdas
or a member pointer that still resolves. Binding the new calls needs a python-enabled
build, which the testing preset does not produce, and is not done here.
Three Python tests mirror C++ cases whose expectations moved when the inverted time interpolation was fixed, so they carried the same wrong values: test_unique_time_steps, test_inactive_cell and test_tutorial. The new values are transcribed from the C++ source, where each first step was derived by hand rather than captured from the runner. test_max_tracing_distance failed for a different reason worth recording. Its positions matched to six decimals while one *time* differed by 4.4e-16 -- one ULP -- because the times were compared with assert_array_equal, exact float equality, while the positions in the same test were already compared approximately. Bisecting placed it on the interpolation fix, which was not the obvious answer: that test supplies identical scalars at both time steps, so the two weighted terms are the same pair of products and IEEE addition is commutative. The cause is FMA contraction. The compiler folds `d1 * w1 + d2 * w2` into a fused multiply-add, computing one product exactly inside the FMA and rounding the other; swapping which weight multiplies which time step therefore moves the last bit even though the mathematics is unchanged. So the assertion was pinning the compiler's contraction decision rather than the tracer's behaviour. All fifteen of these time comparisons had the same latent fragility and only one happened to trip; they now compare approximately, matching what the same tests already do for positions and what the C++ mirrors do. The reason is recorded in the class docstring so a future reader does not tighten them again. The bisect also confirmed something worth having checked: caching the boundary-exit polyline extractor changes no numeric result. The commit that introduced it passes all sixteen Python tests, which the reused GmMultiPolyIntersector could plausibly not have done. flake8 is not installed in this environment, so the Python edit is unlinted.
…t was missing
Adds start_traces, continue_traces, get_trace_results and get_exit_reason to the pybind11
module, exports XmGridTraceExitEnum as exit_reason_enum, and forwards all four through the
hand-written wrapper so FlowPathService can drive the resume loop from Python:
tracer.start_traces(seeds, seed_times)
while tracer.continue_traces() > 0:
step = series.next()
if step is None:
break
tracer.add_grid_scalars_at_time(*step)
traces, times, reasons = tracer.get_trace_results()
continue_traces releases the GIL, and only it. Tracing tens of thousands of seeds takes long
enough that holding the GIL would stall the interpreter for a caller on a worker thread,
which is exactly how this is meant to run. start_traces keeps the GIL because it converts
Python iterables inside its lambda.
start_traces raises ValueError when the start times do not match the points. The C++ side
refuses the batch and returns empty, which from Python would look like a tracer that silently
did nothing.
GetExitReason is added here rather than earlier because it was never actually added. The
commit that claimed it used a scripted string replacement with no assertion that the anchor
matched, so the edit silently did nothing; the build then passed because nothing had changed,
and the claim went unverified into that commit message. It is now on the interface, the impl,
and covered by a test that asserts the single-point path and the batch report the same reason
for the same seed -- if those can disagree, a caller cannot use them interchangeably.
The lesson is in the tooling, not the code: scripted edits need an assertion that the anchor
was found, and a green build is not evidence that an edit landed.
Three Python tests cover the new surface: a trace that continues across three time steps and
whose stopped-early result is a prefix of the continued one, the ValueError, and the batch
matching serial trace_point calls.
Two clang-format suggestions are deliberately not applied. It wants GetExitReason collapsed
to one line, along with all nine sibling accessors that are not written that way; and it wants
425 of the 437 lines of XmGridTrace_py.cpp reformatted, a file never kept under clang-format.
Both would bury the change in unrelated churn.
Also corrects the previous commit's claim that the Python test edit was unlinted -- flake8 is
installed now and it is clean. The only findings in _package are eight pre-existing AQU104
import-header comments, four of them in a file this branch never touched.
flake8 reported eight AQU104 findings across the two Python source files, all on line 1: neither carried the numbered import group comments the Aquaveo rules require. They are pre-existing -- four of them are in grid_trace.py, which this branch had not otherwise touched -- and were invisible until flake8-aquaveo was installed. Plain flake8 only runs pycodestyle, pyflakes and mccabe, so the AQU rules, the google docstring convention and the appnexus import order the .flake8 config asks for were all silently unchecked. The convention, matched from next_ms, is all four comments present even when a section is empty, with a blank line after the module docstring. No import moved, so the suite passing is confirmation the modules still resolve the same way. _package is now clean under the full rule set.
…er sample
GetVectorAtLocationAndTime ran four ExtractData calls per sample -- x and y, for each of two
time steps -- and each one performed its own point-location query for the same (x, y). The
interpolation weights were identical across all four; only the scalar array being weighted
differed.
It now runs one search per distinct triangulation and applies the resulting weights to each
component. Three things make that possible, and the third is the one that had to be measured
rather than assumed:
- The x and y extractors of a time step now share a triangulation, via the sharing
constructor that has existed since 2022 and was simply never used here.
- Both time steps share one as well when their activity masks are equal. The triangulation
and its R-tree depend only on the grid and the mask, so an identical mask makes them
interchangeable. Differing activity is the case that genuinely cannot share, so the mask
is compared rather than assumed -- test_inactive_cell covers that path and
test_unique_time_steps covers the shared one.
- iApplyWeights reproduces ExtractData exactly, accumulating in double and narrowing to
float, so this is bit-identical rather than merely close. Every recorded baseline in both
the C++ and Python suites is unchanged, which is the evidence for that.
Ordering matters in AddGridScalarsAtTime and is easy to get backwards: the y extractor is
built from x, and only after x's scalars are set. The sharing constructor copies the
triangulation and the flag saying what it was built for, so copying x before it has built one
leaves y believing it must build, and y then rebuilds the very triangulation it is sharing --
silently costing what the sharing was meant to save.
Measured on a 200x200 grid at 10,000 seeds, against the previous commit:
before after
setup, 2 time steps 117 ms 32 ms
searches per seed 97.5 24.4
interior us/seed 50.8 12.6
mixed us/seed 50.7 11.8
The setup figure is better than the ~53 ms projected in TRIANGULATION_SHARING.md. That
projection assumed a per-extractor scalar-handling floor of roughly 8 ms, derived by
subtraction rather than timed; the measurement says it is far smaller, and that only one
triangulation and one R-tree are now built for all four extractors rather than four of each.
A re-trace of 10,000 glyphs is now about 150 ms including setup, against the 299 ms the
current in-render tracer costs for the same work -- so the new path is faster than what it
replaces while adding the time dimension, rather than merely close enough. It began this
branch at about 21 seconds.
Also fixes a latent crash this rewrite made obvious: with only one time step supplied, the
first extractor is null and was dereferenced. It now returns a clean extraction failure,
which is what edge case 9 in the session plan assumed already happened.
The benchmark's counter counted ExtractData calls and now counts searches, which is what it
always meant; its label and field are renamed to match rather than silently changing meaning.
A resumed trace could hang, a legal pair of time steps could read out of bounds, and a staggered seed could be killed off permanently. All three are in code this branch introduced. StepTrace hung when a window ended exactly on the second time step. The time step clamp computes deltaT = m_time2 - elapsed - ptTime, which for a trace already sitting on m_time2 is exactly zero, and that zero was persisted into the resumed trace. A zero-length step moves nothing and changes no velocity, so no clamp and no subdivision test could ever end the loop -- and the min-delta-time escape is inside the split branch, which a zero-length step can never enter. It spun forever appending nothing, with the GIL released so Python could not interrupt it. The window is now finished before stepping, keeping the step size the call came in with, so a redundant ContinueTraces really is the no-op the header promises. StepTrace also floors a non-positive resumed step size, so no path can reintroduce this. AddGridScalarsAtTime decided triangulation sharing from the activity mask alone. The triangulation is built for a data location -- LOC_CELLS adds a centroid per cell, LOC_POINTS adds none -- and sharing shares the object rather than copying it, so the second step's SetGrid*Scalars rebuilt the triangulation the first step was still pointing at. Its shorter scalar array was then indexed by the new centroid indices: an out-of-bounds read, not a wrong answer. Both data locations now join the mask in the predicate. A seed released after the loaded window reported GTEXIT_EXTRACTION_FAILED, which iIsTerminal treats as terminal, so the seed never started even once its time step arrived. StartTraces takes a release time per seed so a batch can be staggered, making this an ordinary input; it now reports GTEXIT_WAITING_FOR_TIME_STEP, as the mid-trace clamp always did. Adds a regression test per fix. testBeyondTimestep and its Python twin asserted only that the trace was empty, which is why the third defect went unnoticed -- they now assert the exit reason, which is what tells the three empty-trace cases apart. C++ 25/25, Python 19/19, flake8 clean.
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.
Fast-forwards
masteronto the line development has actually been happening on.masteris 14 behind and 0 ahead, so nothing here is a divergence to reconcile — its tip is still "fix install script" from seven years ago.Fourteen commits in two groups.
Build and CI modernization (4 commits)
Migrates to xmsconan 2.15.2 → 2.15.4 → 2.16.0: the
build.tomllayout, the_package/Python package, regenerated CMake/conan files and GitHub Actions workflows, an xmsextractor bump to 10.0.6, and the API drift that bump exposed. Deletes.appveyor.yml— AppVeyor and Travis webhooks are already removed from the repository, so those contexts no longer report.Time-stepped tracing (10 commits, merged as #10)
XmGridTracegains a resumable batch API —StartTraces/ContinueTraces/GetTraceResults— so a trace suspends at the second loaded time step and continues when a later one arrives, carrying its position, budgets, adaptive step size and previous velocity across the boundary. The two-step window is retained, so memory stays bounded however long the series is. Exit reasons are now anXmGridTraceExitEnumper trace rather than a composed string.Four correctness fixes ride along, all in code paths that predate this work:
t == t1the particle was advected entirely by t2's field-9999999sentinel into a real-looking velocityTracePoint's position and time arrays could come back different lengthsPerformance, measured on a 200x200 quad grid at 10,000 seeds: a re-trace went from ~21 s to ~150 ms, against the ~300 ms the current in-render tracer costs for the same work without a time dimension. Two changes account for it — caching the boundary-exit extractor instead of rebuilding it per exit event, and one point-location search per triangulation instead of four per sample. Both are contained in this repo; no xmsextractor change and no version cascade.
Three further defects were found by review of #10 and fixed there before merge: a resumed trace could hang when a window ended exactly on a time step, a legal pair of time steps at different data locations could read out of bounds, and a seed released after the loaded window was killed off permanently rather than made to wait.
Verification
Re-run on this branch tip, not inherited from #10:
./build/Release/runner)python build.py --python-only)flake8 _package/clean#10's 18 GitHub Actions jobs passed on identical content —
git diffbetween the tested tree and this branch tip is empty.Note for the reviewer
This is a fast-forward of a long-dormant
master, so the diff against it is large by construction. #10 carries the detailed review of the tracer half; the build half is unreviewed only in the sense that it was never separately PR'd.