Guard omp_lib so serial and non-GNU builds compile - #514
Conversation
Three call sites gate orbit classification: the bmin/bmax cache that separates trapped from passing, the dispatch to trace_orbit_with_classifiers, and the class_parts.dat writer. All three tested (ntcut > 0 .or. class_plot) and ignored fast_class, which is read only inside the classifier routine. So fast_class = .True. with tcut <= 0 and class_plot = .False. traced ordinary orbits, wrote no classification output, and left the flag inert, though it documents "quit immediately after fast classification". That combination is the only one that classifies without the Minkowski fractal cut: the cut fires at kt == ntcut, and the early exit in classification.f90 requires .not. class_plot. Without it, fast classification always costs either the fractal path or full-length traces. Extract the shared predicate as params.classification_enabled and include fast_class in it. Existing configurations are unaffected: both golden records set class_plot = .True., so the predicate keeps its value there.
The classifier forms two continuous quantities and then discards them. The J_parallel class thresholds the spread of the parallel invariant across banana tips against tol_perpinv; the ideal-orbit class is the sign of the minimum monotonicity margin over the ordered tip sequence. Only the integer codes survive into class_parts.dat. Anything that wants a smooth confinement score has to reconstruct those quantities by re-tracing, and any attempt to differentiate a metric built on the integer codes gets exactly zero, silently, because the codes are piecewise constant. Keep both quantities and write them to class_scores.dat: J_parallel spread, the reference magnitude it should be compared against, the monotonicity margin, the score status, and trap_par. class_parts.dat keeps its column contract, so existing readers and both golden records are untouched. The classifier loops no longer exit early, since a maximum and a minimum need every term. That costs a few comparisons per resolved orbit and leaves the integer classes bit-identical. score_status distinguishes the cases a consumer must not conflate: 1 means the topology margin is a real margin, 2 an early stochastic exit that forms neither quantity, 3 a resolved orbit whose ideal class came from the recurrence test, which produces no margin. The J_parallel spread is valid for 1 and 3.
The class margins are thin on real runs. On a reactor-scale candidate at nturns = 8 only 2 of 1024 orbits carried a J_parallel spread and none carried a monotonicity margin, because both need the classifier to complete nturns return periods of the tip map, which a 20 ms trace rarely reaches. A score with no data cannot steer an optimizer. The tip sequence carries a quantity that needs none of that: the radial excursion max(s) - min(s) over the banana tips collected so far. Two tips suffice, no threshold, no recurrence, no nturns. It is the width of the radial band the orbit explores, which is exactly what a phase-space barrier suppresses, so it is the natural continuous confinement measure rather than a mollified class code. The driver test now exercises it on 22 orbits where the class margins reach 4 and 0, and checks the pairing both ways: two tips imply a positive excursion, fewer than two imply none.
Six files referenced omp_lib without the `!$` sentinel, so the build broke outright whenever the module was absent: ENABLE_OPENMP=OFF on any compiler, and every flang build on a toolchain whose OpenMP runtime ships no omp_lib.mod. The failure was a hard semantic error, not a warning. Each site now has a serial fallback. simple_main uses timing's get_wtime(), which already documents itself as the omp_get_wtime replacement, and the two debug prints take the thread id through a guarded local. simple_gpu defaults its device index to 0, matching the single-device path it already takes when ngpu <= 1. test_openmp_optional.py rejects any unguarded reference. Its oracle is the Fortran rule that such a line cannot compile without OpenMP, so it constrains files that do not exist yet; it reports 13 findings on the parent commit. Verified: gfortran with OpenMP unchanged, flang 23 with OpenMP builds all 1209 targets, and the two agree bit-for-bit (0.0e+00 on loss times, final coordinates and confined fraction) on a Boozer-field case at 8 threads with pinned starting conditions.
krystophny
added
tier/T3
|
Test suite state, clean build tree ( 109/116 pass. All 7 failures reproduce on the parent commit
One worth flagging separately: |
![slopqueue[bot]](https://avatars.githubusercontent.com/u/48486978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review verdict: Approve
Summary: This PR (4 commits) makes the classifier's continuous decision inputs available for downstream confinement scoring and fixes two real defects. (1) omp_lib references are guarded with the !$ sentinel so serial/non-GNU builds compile (verified by a new test_openmp_optional.py lint that I ran locally — clean across all 173 sources). (2) fast_class alone now enables classification dispatch via a new classification_enabled() predicate in params.f90, fixing the bug where fast_class=True with tcut<=0, class_plot=.F. silently traced ordinary orbits. (3) class_scores.dat exposes jpar_spread, jpar_ref, topology_margin, a score_status flag, and the new tip radial_spread/tip_count, kept in a separate file so class_parts.dat's column contract is preserved. I verified the new predicate against all 10 rows of bminmax_cache_cases.tsv (Fortran and Python both match), confirmed the refactored check_orbit_type monotonicity margin produces byte-identical integer classes (the removed exits were only an early-exit optimization; margin<0 ⟺ ideal=2 holds), and confirmed get_wtime() type-compatibly replaces omp_get_wtime(). Coverage is good (new driver tests + extended lifecycle cases). I could not run the full make test because the environment lacks BLAS/LAPACK.
Findings:
-
[minor] python/pysimple/init.py:472 — the only real caller of
_needs_bminmax_cachestill passes only(params.num_surf, params.ntcut, params.class_plot)and omitsparams.fast_class, even though this PR added thefast_classparameter precisely to keep Python in sync with the Fortranclassification_enabled()predicate. It is currently harmless only because thenum_surf != 1fallback coincidentally yields the same result as the Fortrannum_surf > 1branch for everynum_surf, but it contradicts the PR's own "three call sites must agree" docstring and silently diverges the moment either predicate is touched. Fix: passparams.fast_class(or read it from params inside the function) so the Python cache gate tracks the Fortran dispatch. -
[minor] test/tests/test_class_scores_driver.py:31 — the oracle re-thresholds
jpar_spreadread back from the ASCIIclass_scores.datagainst a hardcodedTOL_PERPINV = 15.0, duplicatingtol_perpinv = 15.d0insrc/check_orbit_type.f90(and the underlyingdrift > tol_perpinvtest). Both use strict>, so borderline values near exactly 15.0 could round across the threshold on a different field/compiler and flake. It passes on the fixed deterministic test field, but deriving the constant from the Fortran source (or testing a threshold-aware interval) would be more robust.
Verdict: Approve — the changes are correct, well-tested, and both stated defects (serial-build breakage and fast_class silently doing nothing) are genuinely fixed; the two findings above are non-blocking robustness/consistency notes.
1 participant
Problem
Six files referenced
omp_libwithout the!$sentinel. That is a hard semantic error, not a warning, whenever the module is absent:-DENABLE_OPENMP=OFFon any compilerflangbuild on a toolchain whose OpenMP runtime ships noomp_lib.mod(LLVM 22 here does not; LLVM 23 does)Fix
Each site gets the sentinel plus a serial fallback:
simple_maintakes wall time fromtiming'sget_wtime(), which already documents itself as "replacement for omp_get_wtime", and routes the two debug prints through a guarded local thread id.simple_gpudefaults its device index to 0 — the same value the single-device path already implies whenngpu <= 1.classification,test_coord_transimport no symbols, so the sentinel alone suffices.Test
test/python/test_openmp_optional.pyrejects any unguarded reference. Its oracle is the Fortran rule that such a line cannot compile without OpenMP, so it constrains files that do not exist yet rather than pinning current contents. It reports 13 findings on the parent commit and passes on this one.Verification
ENABLE_OPENMP=OFFgfortran and flang-23 agree bit-for-bit on a Boozer-field case (midpoint integrator, 8 threads, pinned
start.dat):0.0e+00max relative difference on loss times, final coordinates and confined fraction, at trace times 6e-5, 1e-3 and 1e-2 s.Incidental finding, not addressed here:
sample_particles_test_fieldignoresstartmodeand callsrandom_number, soisw_field_type = -1cases cannot be compared across compilers — gfortran and flang have different PRNGs. Any cross-compiler check needs a real field withstartmode = 2.