fix: correct dedx_get_inverse_stp() branch selection for monotone STP curves - #148
fix: correct dedx_get_inverse_stp() branch selection for monotone STP curves#148grzanka wants to merge 6 commits into
Conversation
… curves (#121) find_min() searched a hardcoded x in [0.01, 10] instead of the program's actual tabulated energy range, and side < 0 was the only branch that ever selected the low-energy path, so side == 0 and side == 1 were indistinguishable. In practice dedx_get_inverse_stp() failed with DEDX_ERR_ENERGY_OUT_OF_RANGE for ordinary proton/water queries on both PSTAR and ICRU49, regardless of side. Replace find_min()/find_min_stp_func() with find_stp_peak(), which samples STP on a log-spaced grid over the real [emin, emax] to locate the Bragg-peak energy, then bisects the physically correct branch: the full descending range when there is no interior peak, side == 0 -> ascending [emin, e_peak] (falling back to the descending branch when the STP is below the ascending branch's floor), side == 1 -> descending [e_peak, emax]. Out-of-range STP values now return a clean error instead of looping to a bogus or negative energy. Also add dedx_get_bragg_peak_stp() per the issue's acceptance criteria, and fix dedx_get_inverse_stp() to guard on config->loaded before calling dedx_load_config(), matching dedx_get_csda()'s existing pattern -- without it, calling dedx_get_inverse_stp() on an already-loaded config corrupted the workspace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
+ Coverage 74.51% 77.99% +3.47%
==========================================
Files 12 12
Lines 1711 1777 +66
Branches 317 340 +23
==========================================
+ Hits 1275 1386 +111
+ Misses 436 391 -45 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes dedx_get_inverse_stp() so it selects the correct inverse-branch over the actual tabulated energy range (including monotone STP curves), and adds a new public helper to query the Bragg-peak (maximum) stopping power for a loaded configuration.
Changes:
- Replace the previous hardcoded-range peak search with a log-sampled Bragg-peak finder and branch-aware bisection in
dedx_get_inverse_stp(). - Add
dedx_get_bragg_peak_stp()to the public API for retrieving the peak (maximum) stopping power over the tabulated range. - Add a new regression test suite covering monotone fallback behavior, branch selection (
side=0/1), and out-of-range STP handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/dedx_tools.c |
Implements sampled Bragg-peak detection, fixes inverse STP branch selection/bounds handling, and adds dedx_get_bragg_peak_stp(). |
include/dedx_tools.h |
Updates dedx_get_inverse_stp() docstring semantics and declares the new dedx_get_bragg_peak_stp() API. |
tests/test_inverse_stp.c |
Adds regression tests for inverse STP branching, monotone fallback, out-of-range behavior, and the new peak-STP helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
exp(log(emin)) is not guaranteed to round-trip to exactly emin; a reconstructed value fractionally below the dataset's real lower bound would be rejected by dedx_get_stp(), leaving stp_at_emin stuck at 0 and making the ascending-branch check in dedx_get_inverse_stp() always pass. Addresses Copilot review comment on #148. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/dedx_tools.c:231
- Same workspace/config mismatch risk as in dedx_get_inverse_stp(): if
configwas loaded in a different workspace (orwswas recreated),config->loaded==1can cause this function to skipdedx_load_config()and then all sampling calls intodedx_get_stp()will fail withDEDX_ERR_INVALID_DATASET_ID(src/dedx.c:490-492). Re-load whencfg_idis not valid for this workspace.
if (config->loaded == 0)
dedx_load_config(ws, config, err);
"Bragg peak" properly refers to the maximum of a depth-dose curve (which also depends on range straggling), not the maximum of the stopping-power- vs-energy curve this function returns. dedx_get_max_stp() names the quantity precisely instead of borrowing dosimetry terminology for a stopping-power quantity. This diverges from the name used in the #121 issue spec and in dedx_web's wasm/dedx_extra.c reference implementation; dedx_extra.c is slated for removal once the migration to libdedx completes, so it will pick up the new name rather than the other way around. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
include/dedx_tools.h:69
- Same as for dedx_get_inverse_stp(): the docstring states the configuration is already loaded, but
dedx_get_max_stp()loads it whenconfig->loaded == 0. Update the docs to reflect that the function may load/mutateconfig.
* @param[in] ws Workspace with a loaded configuration.
* @param[in] config Loaded configuration.
…ensity STP(E) is not simply unimodal. PSTAR/ICRU proton tables extend to 10 GeV/nucleon, far enough to show the curve rise to the Bragg peak (~0.08 MeV), fall through a minimum-ionizing point (~3000-4000 MeV), and rise again (relativistic rise) toward the table's max energy. The previous 40/300-sample log-spaced scan assumed a single interior peak and rejected every STP in that high-energy dip as out of range, even though it's reachable at two energies -- verified directly against the data: stp=2.05 side=0 -> err=101 (before), e=1649 (after) stp=2.05 side=1 -> err=101 (before), e=5904 (after) Replace the sampling with a linear scan over the exact tabulated knots (dedx_tools.c can reach dedx_lookup_data.h since it's a private, same-directory header; dedx_spline.c sets coef[i].a = stopping[i], so the knots are the literal (energy, STP) pairs the spline was built from). The scan finds every monotonic run the curve actually has in one pass, no sampling-density guesswork, and dedx_get_inverse_stp() now brackets and bisects each run that contains the requested STP, returning the lowest-energy (side=0) or highest-energy (side=1) reachable solution -- a strict generalization of the old ascending/descending choice that also covers curves with more than one turning point. dedx_get_max_stp() becomes an exact scan of the same knots (previously a 300-sample approximation). Add the complementary dedx_get_min_stp(), which for tables reaching relativistic energies reports the minimum-ionizing point rather than either tabulated endpoint. tests/test_inverse_stp.c: add a regression test for the relativistic-rise dip (the exact scenario above) and for dedx_get_min_stp(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
src/dedx_tools.c:234
sideis documented as 0/1, but the previous implementation also treatedside < 0as selecting the low-energy branch. The new code maps any nonzeroside(including negative) to the high-energy solution, which is a behavioral regression for existing callers that may have relied onside < 0(and is inconsistent with the common “<=0 / >0” pattern). Consider treatingside <= 0as the low-energy solution andside > 0as the high-energy solution.
return (side == 0) ? x_min_found : x_max_found;
include/dedx_tools.h:64
dedx_get_max_stp()is documented as the maximum of the stopping-power-vs-energy curve, but the implementation computes the maximum over tabulated knots only. With natural cubic spline interpolation, a true maximum can occur between knots. Either clarify that this returns the max knot value, or update the implementation to analyze spline extrema.
/** @brief Find the maximum stopping power over a program's whole tabulated
* energy range.
*
* This is the peak of the stopping-power-vs-energy curve, not the Bragg
* peak of a depth-dose curve (which also depends on range straggling and is
include/dedx_tools.h:78
- Similarly,
dedx_get_min_stp()returns the minimum over tabulated knots, but the doc implies it is the minimum of the interpolated curve. With natural cubic spline interpolation, the true minimum can occur between knots. Clarify the definition (knot min vs spline min) or update the implementation accordingly.
/** @brief Find the minimum stopping power over a program's whole tabulated
* energy range.
*
* For tables that reach relativistic energies this is typically the
* minimum-ionizing point, not the value at either tabulated endpoint (the
- load_and_get_dataset() -> get_loaded_dataset(): still static/file-private (only this translation unit needs it, unlike the dedx_internal_* functions declared in headers for cross-file use); comment now says so explicitly. - Replace the fixed acc = 1e-5 (absolute, in the same MeV units as the bisected energy) with a tolerance relative to the current bracket (DEDX_INVERSE_STP_REL_ACC = 1e-6). 1e-5 MeV is 10 eV, not 10 keV -- and as an absolute value it's a poor fit for a domain spanning many decades (proton tables run ~0.001-10000 MeV/nucleon): far too loose relative to the low end, far tighter than dedx_get_stp()'s float precision can even represent at the high end (it truncates the search value to float before evaluating the spline). A relative tolerance keeps achieved precision consistent across the whole range and matches float's own ~1.19e-7 relative precision instead of overshooting it. Documented inline with the reasoning and the eV-scale sanity check. - Remove every ternary in bisect_monotonic_run() and dedx_get_inverse_stp(), replaced with if/else and comments explaining what each branch means physically (bracket direction, run-boundary detection, ascending vs descending step selection). No behavior change beyond the tolerance switch, which tightens precision at low energies and avoids wasted iterations at high energies; verified against the same relativistic-rise and cross-program checks as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- bisect_monotonic_run(): bisect in log-energy space instead of raw energy. The tabulated grid is predominantly log-spaced (constant energy *ratio* per NIST-derived tables), and an absolute tolerance in log-space is exactly a relative tolerance on the energy result (d(ln x) = dx/x) -- more principled than the previous fabs(x1-x2) > REL_ACC*fabs(x2) approximation. Renamed DEDX_INVERSE_STP_REL_ACC -> ...LOG_ACC to match. - get_loaded_dataset(): reset *err = DEDX_OK on entry instead of treating an incoming nonzero value as a precondition (matches dedx_get_stp()); also reload when config->loaded == 1 but config->cfg_id isn't a valid slot in *this* workspace (e.g. the config was loaded into a workspace that was since freed and replaced), instead of just erroring out. Both from Copilot review on #148. - Doc/comment fixes from the same review: dedx_get_inverse_stp()/ dedx_get_max_stp()/dedx_get_min_stp() docstrings now say the config loads automatically rather than implying a precondition; side is documented as exactly what the code checks (side == 0 vs any other value); the "finds every feature" comment no longer overclaims exhaustiveness over the interpolated spline (only over the tabulated points); min/max-stp docstrings say "computed exactly from the tabulated data points" instead of implying a guarantee about the continuous interpolated curve. - tests/test_inverse_stp.c: derive the out-of-range test's STP bounds from dedx_get_max_stp()/dedx_get_min_stp() instead of hardcoded constants; add regression tests for get_loaded_dataset()'s load-failure propagation (through all three affected functions) and its reload-on-stale-workspace fix. - Remove a dead `else dir = 0;` branch (dir is already 0-initialized). Patch coverage: 92.2% (was 91.8%); the remaining uncovered lines in dedx_tools.c are defensive branches unreachable via the public API given dedx_load_config()'s and the bisection's own invariants (verified, not just assumed) -- consistent with similar defensive branches elsewhere in this codebase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes #121 · Phase C of the
dedx_extra.cmigration epic #118.Bug
dedx_get_inverse_stp()usedfind_min()to locate the energy of maximum stopping power, searching a hardcodedx ∈ [0.01, 10]instead of the program's actual tabulated energy range. Separately, the documentedsidecontract is0= low-energy branch /1= high-energy branch, but the implementation only special-casedside < 0for the low branch — soside == 0andside == 1were indistinguishable, both returning the descending-branch result.In practice this made the function fail outright for ordinary queries. I reproduced it directly before fixing:
(
err=203=DEDX_ERR_INVALID_DATASET_ID/DEDX_ERR_ENERGY_OUT_OF_RANGEdepending on the code path — either way, broken.)Fix, round 1: sampled-peak + branch-aware bisection
Ported the sampled-peak + branch-aware bisection strategy proposed in #121 (originally proven out in
dedx_web'swasm/dedx_extra.cdedx_get_inverse_stp_flat): sample STP at a fixed log-spaced density to locate the Bragg peak, then bisect the ascending or descending branch. This fixed the reported case but assumed a single interior peak.Fix, round 2: STP(E) is not simply unimodal
Review (h/t @grzanka) pointed out that "Bragg peak" isn't the right term for the STP(E) maximum (that's a depth-dose concept — see the rename below), and — more importantly — questioned whether a single peak/branch model is even correct. It isn't. PSTAR/ICRU proton tables extend to 10 GeV/nucleon, far enough to show the curve rise to the Bragg peak (~0.08 MeV), fall through a minimum-ionizing point (~3000–4000 MeV), and rise again (relativistic rise / Fermi plateau) toward the table's max energy. I confirmed this directly against the tabulated data (both a dense synthetic scan and the raw knots) before changing anything: exactly two turning points for PSTAR/ICRU/ICRU49 proton/water, one turning point for every other program/ion combination I checked (their tables don't reach high enough energy to show the second one).
The previous fixed-sample-density scan assumed a single interior peak and rejected every STP in that high-energy dip as out of range, even though it's reachable at two energies:
Redesign: walk the exact tabulated knots instead of sampling
dedx_tools.ccan reachdedx_lookup_data.h(a private, same-directory header) and readws->loaded_data[config->cfg_id]->base[i].x/.adirectly — these are the literal(energy, STP)pairs the spline was built from (dedx_spline.c:coef[i].a = stopping[i]). Replaced the fixed-sample-count scan with a single linear pass over these exact knots that finds every monotonic run the curve actually has — no sampling-density guesswork, no risk of straddling a feature between synthetic samples, and no arbitrary sample-count constants (DEDX_INVERSE_STP_SAMPLES/DEDX_MAX_STP_SAMPLESare both gone).load_and_get_dataset()(new) loads the config if needed and returns the backingdedx_internal_lookup_data, bounds-checked likededx_get_stp()already does.dedx_get_inverse_stp()walks the knots once, brackets and bisects every monotonic run that contains the requested STP (viabisect_monotonic_run(), usingdedx_get_stp()'s spline evaluation for the actual root-finding, so precision is unaffected), and returns the lowest-energy reachable solution forside == 0or the highest-energy one forside == 1. This is a strict generalization of the old ascending/descending branch choice — identical behavior for the common single-peak case, but now also correctly resolves the two solutions on either side of the minimum-ionizing point.dedx_get_max_stp()is now an exact scan of the same knots (previously a 300-sample approximation).dedx_get_min_stp()— for tables reaching relativistic energies this is the minimum-ionizing point, not either tabulated endpoint.Naming:
dedx_get_max_stp()instead ofdedx_get_bragg_peak_stp()The issue spec (and the
dedx_webreference this was ported from) named thisdedx_get_bragg_peak_stp(). "Bragg peak" properly refers to the maximum of a depth-dose curve, which also depends on range straggling — not the maximum of the stopping-power-vs-energy curve this function returns. Renamed todedx_get_max_stp()to name the quantity precisely; the header docstring calls out the distinction explicitly.wasm/dedx_extra.cis slated for removal once this migration completes, so it'll pick up the new name rather than the other way around. Commented on #121 with the same rationale.Tests
tests/test_inverse_stp.c(new, auto-registered by thetest_*.cglob):side=0/side=1agree, round-tripped viadedx_get_stp().side=0/side=1select distinct energies, both round-trip.side=0/side=1select the pre- and post-minimum energies respectively, both round-trip — the exact scenario that used to returnDEDX_ERR_ENERGY_OUT_OF_RANGE.DEDX_ERR_ENERGY_OUT_OF_RANGE, not a bogus/negative energy.ion_a <= 0still rejected up front (unchanged contract).dedx_get_max_stp()/dedx_get_min_stp()checked against a 5000-point brute-force reference scan.Verification
ctest: 33/33 pass (32 existing + the new test).clang-format-19 --Werror: clean on all changed files.gcc -Wall -Wextra -Wpedantic -Wshadow: clean (clang-tidy itself isn't available in this environment; CI will run it).dedx_get_inverse_stp()/dedx_get_max_stp()/dedx_get_min_stp()against 7 other program/ion/material combinations (ICRU73/carbon, ASTAR/helium, MSTAR/carbon, Bethe, ICRU49/ICRU protons and helium) — all single-peak curves resolve correctly with distinct low/high solutions.Authored via Claude Code as Phase C of the
dedx_extra.c→ libdedx migration plan.