Fix int32 overflow in ntcut that hijacks long traces into the classifier#434
Merged
Conversation
ntcut = ceiling(ntimstep*ntau*tcut/trace_time) evaluates ntimstep*ntau as a 32-bit product. For second-scale traces ntau reaches ~2.3e6 (npoiper2=16384, ntau = ceiling(dtau/dtaumin)), so ntimstep*ntau = 1000*2.3e6 = 2.3e9 exceeds 2^31, wraps negative, and times tcut<0 flips ntcut positive. ntcut>0 then routes the whole run through trace_orbit_with_classifiers instead of the normal macrostep path, changing loss times and the confined fraction. Threshold is trace_time ~ 0.94 s at npoiper2=16384; shorter traces are unaffected. Evaluate the product in real(dp) and return int64 via microstep_cut_index; tcut<=0 now yields ntcut=-1 (classification off) for any trace length. Add test_ntcut_overflow.
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.
Problem
params_initcomputes the classification cut index asntimstep*ntauis an int32 * int32 product evaluated before the realmultiply. For second-scale traces
ntaureaches ~2.3e6 (npoiper2=16384,ntau = ceiling(dtau/dtaumin)), sontimstep*ntau = 1000 * 2.3e6 = 2.3e9exceeds 2^31, wraps negative, and
* tcut(-1)flipsntcutpositive.ntcut > 0then routes the entire run throughtrace_orbit_with_classifiersinstead of the normal
macrosteppath, silently changing loss times and theconfined fraction. Threshold is
trace_time ~ 0.94 sat npoiper2=16384;shorter traces never reach the overflow and are unaffected.
Found while benchmarking the W7-X high-mirror reactor case: a 1 s GC run gave
cf@100ms = 0.756vs the correct0.856from a 100 ms run at the samephysical time, with everything but
trace_timeidentical.Fix
Extract
microstep_cut_index, evaluate the product inreal(dp), returnint64, and disable classification (ntcut = -1) whenevertcut <= 0, for anytrace length.
ntcutbecomesinteger(8)(the comparisonkt == ntcutalreadyuses
integer(8) kt).Verification
Exact arithmetic, before (int32, as on main) vs after (int64):
New regression test (would not compile/pass against the int32 path):
Full fast suite on this branch:
make test-fast-> 55/55 passed, 0 failed.Single-particle W7-X reactor check (deterministic, GC, npoiper2=16384): particle
loss time at
trace_time=0.1andtrace_time=1.0was 168.348 us vs 246.019 usbefore, and 168.348 us in both after;
class_parts.datis no longer written forthe 1 s run.
Golden records: untouched. They use short traces (no overflow), so the cut index
is bitwise-identical; the fix changes results only for
trace_time >~ 0.94 s.