Repository navigation
Fix stale material-availability tables; add ion-aware lookup and Bethe fallback for DEDX_ICRU - #144
Merged
Merged
Conversation
…e fallback for DEDX_ICRU dedx_program_available_materials falsely claimed elements (Boron, Nickel, Zirconium, Gadolinium, Tantalum as applicable) as available for ASTAR, PSTAR, ICRU49, ICRU73 and ICRU73_OLD when the embedded data for those programs never tabulated them, e.g. HYDROGEN + PSTAR + BORON (#51). The rows are corrected against the actual embedded target-id tables; MSTAR, which genuinely tabulates all of these, is left untouched. Add dedx_get_material_list_for_ion() / dedx_fill_material_list_for_ion(), which answer "what materials work with this program and this ion" by consulting the same embedded-data resolution dedx_load_config() uses, including Bragg-additivity decomposition of compounds -- so the answer can't drift out of sync with the underlying data the way the hand-maintained table did. DEDX_ICRU (the auto-selecting report) now falls back to the Bethe-Bloch formula for elements no tabulated report covers, instead of failing with DEDX_ERR_COMBINATION_NOT_FOUND. Report-specific programs (PSTAR, ASTAR, ICRU49, ICRU73, ...) intentionally keep failing those combinations rather than silently blending in another program's data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
+ Coverage 72.86% 74.13% +1.26%
==========================================
Files 12 12
Lines 1640 1697 +57
Branches 299 315 +16
==========================================
+ Hits 1195 1258 +63
+ Misses 445 439 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #51 by correcting previously-stale per-program material availability tables, introducing an ion-aware material lookup API that mirrors dedx_load_config() resolution (including Bragg additivity), and extending DEDX_ICRU to fall back to a Bethe-Bloch calculation for elemental targets not covered by any tabulated ICRU/PSTAR/ASTAR dataset.
Changes:
- Fix stale
dedx_program_available_materialsrows so restricted programs no longer advertise elements not present in embedded*_target_ids. - Add
dedx_get_material_list_for_ion()/dedx_fill_material_list_for_ion()for accurate program+ion material availability queries. - Add
DEDX_ICRUBethe-Bloch fallback when no tabulated report covers an elemental target.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_material_availability.c | Adds regression tests for corrected material tables, new ion-aware material lookup, and DEDX_ICRU Bethe fallback behavior. |
| src/dedx.c | Implements ion-aware material support checks and DEDX_ICRU Bethe fallback logic in find_data(). |
| src/dedx_wrappers.c | Adds wrapper dedx_fill_material_list_for_ion() to produce a -1-terminated list for callers. |
| src/dedx_program_const.h | Updates per-program material lists to match embedded target-id coverage; adds guidance comment about ion-aware API. |
| include/dedx.h | Documents DEDX_ICRU fallback behavior and adds the new dedx_get_material_list_for_ion() API + buffer-size macro. |
| include/dedx_wrappers.h | Documents the new wrapper API for ion-aware material listing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DEDX_ICRU and DEDX_AUTO are different models and shouldn't be conflated: DEDX_ICRU stays a real, literal, tabulated auto-select between ICRU49/ICRU73 (no fallback), while the new DEDX_AUTO program layers a Bethe-Bloch tier on top for elements no tabulated report covers -- comparable to the AUTO mode in dedx_web. DEDX_AUTO is deliberately grouped with the other tabulated report identifiers (not >= DEDX_DEFAULT) so a compound target still resolves through the same tabulated-first path as DEDX_ICRU instead of always decomposing into elements, which was the case in an earlier draft of this change and gave DEDX_AUTO wrong stopping powers for directly-tabulated compounds like water. Also addresses Copilot's review comments on PR #144: reserves the terminator slot in dedx_fill_material_list_for_ion() and its test, clarifies the DEDX_MAX_MATERIAL_LIST/output-format docs, and rejects ions the periodic table doesn't actually cover (>112) for the Bethe-capable programs in dedx_get_material_list_for_ion(), consistent with what dedx_load_config() does. Bumps DEDX_MAX_MATERIAL_LIST to 1000 per request. Closes the codecov patch-coverage gaps: two of the added guards are truly unreachable given today's embedded data (verified by probing every candidate element/compound) and are marked LCOV_EXCL with a note why; the rest needed a DEDX_MSTAR ion-remap test case and fell out of removing now-dead i-value-fetch logic that direct testing showed was never reachable in the first draft. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Clarify what element_supported_for_ion(), material_id_supported(), and load_bethe_fallback() return, for readers who only skim the function signatures. Comment-only change, no behavior difference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines
+627
to
630
| if (prog >= DEDX_DEFAULT || prog == DEDX_AUTO) { | ||
| if ((ion < 1) || (ion > 120)) | ||
| return 0; | ||
| return 1; |
Copilot pointed out that check_ion() accepted ions up to 120 for DEDX_DEFAULT/DEDX_BETHE_EXT00/DEDX_AUTO, wider than the 1-112 range the periodic-table data backing the Bethe-Bloch formula actually supports. Tightened check_ion() itself to 1-112 (matching dedx_get_ion_list()) and dropped the now-redundant duplicate check in dedx_get_material_list_for_ion(). Verifying that surfaced a worse, pre-existing bug: for DEDX_DEFAULT/ DEDX_BETHE_EXT00 with a compound target, the compound decomposes into elements before check_ion() ever runs, so an invalid ion had to be caught inside load_bethe_2() instead -- and it wasn't. dedx_internal_get_atom_charge()/ get_atom_mass() each reset *err to DEDX_OK on entry, so the ion's DEDX_ERR_NOT_AN_ELEMENT was silently overwritten by the (valid) target element's successful lookup right after it, yielding a "successful" load with a NaN stopping power. Fixed by checking *err right after the ion's charge/mass pair, before the target's pair can clobber it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Closes #51.
dedx_program_available_materialsclaimed elements (Boron, Nickel, Zirconium, Gadolinium, Tantalum, as applicable per program) as available for ASTAR, PSTAR, ICRU49, ICRU73 and ICRU73_OLD, when the embedded data for those programs never actually tabulated them — e.g. the reportedHYDROGEN + PSTAR + BORON. Verified the corrected rows directly against the embedded*_target_idsarrays; MSTAR, which genuinely tabulates all of these, is left untouched.dedx_get_material_list_for_ion(program, ion, ...)/dedx_fill_material_list_for_ion(...)answer "what materials work with this program and this ion" by consulting the same embedded-data resolutiondedx_load_config()uses at calculation time (including Bragg-additivity decomposition of compounds into elements). Since it derives the answer from the real data instead of a hand-maintained list, it can't drift out of sync the waydedx_program_available_materialsdid. Thread-safe (writes into a caller-supplied buffer, no shared/static state), matching the direction taken in Fix data race in dedx_get_ion_list() for unrestricted programs #139.DEDX_AUTOprogram with a Bethe-Bloch fallback: per the discussion on #51, report-specific programs (PSTAR, ASTAR, ICRU49, ICRU73,DEDX_ICRU, ...) should keep doing only what they were originally intended to —DEDX_ICRUand a Bethe-Bloch fallback are different models and shouldn't be conflated, soDEDX_ICRUitself is untouched and still fails a combination no tabulated ICRU sub-report covers. Instead there's a new, separateDEDX_AUTOprogram (comparable to the AUTO setting in dedx_web): it auto-selects the best tabulated report exactly likeDEDX_ICRU, but falls back to the Bethe-Bloch formula for elemental targets nothing tabulated covers (e.g. Boron), instead of failing outright. This also makes compounds containing such elements (e.g. Boron Carbide) resolvable via the existing Bragg-additivity path, since each constituent element goes through the same fallback.DEDX_AUTOis deliberately numbered alongside the other tabulated-report identifiers (not>= DEDX_DEFAULT) so a compound target still resolves through the same tabulated-first path asDEDX_ICRUinstead of always decomposing into elements — an earlier draft got this wrong and silently gaveDEDX_AUTOa different (Bragg-decomposed) answer thanDEDX_ICRUeven for directly-tabulated compounds like water.Context
Cross-referencing
APTG/dedx_web#844, which hit exactly this bug from the frontend (proton + Boron in the Elements picker) and traced it back to this same stale table. That issue is scoped to the frontend's defensive handling; this PR is the libdedx-side fix it was waiting on, plus the new ion-aware API andDEDX_AUTOprogram the frontend can use for accurate picker/auto-select behavior once adopted there.Test plan
tests/test_material_availability.c: corrected static tables no longer claim Boron for PSTAR/ASTAR/ICRU49/ICRU73/ICRU73_OLD/ICRU (while confirming MSTAR and ICRU73's genuine Nickel entry are untouched); PSTAR/ICRU73/ICRU still reject Boron atdedx_load_config()time;DEDX_AUTOsucceeds for Boron directly and via Boron Carbide decomposition (bragg_used == 1);dedx_get_material_list_for_ion()/dedx_fill_material_list_for_ion()checked across ASTAR (ion mismatch), MSTAR (ion-remap path), ICRU vs AUTO, ICRU73, ESTAR (not implemented), and an out-of-periodic-table-range ion for AUTO.clang-format-19 --dry-run --Werrorclean on all changed files.gcovr/lcov's coverage preset that every added/changed line insrc/dedx.candsrc/dedx_wrappers.cis exercised by a test, except two data-boundary guards confirmed unreachable by probing every real embedded element/compound (markedLCOV_EXCLwith a note why, matching the project's existing convention).DEDX_ICRUandDEDX_AUTOgive identical stopping powers for a directly-tabulated combination (proton + water), confirmingDEDX_AUTOprefers tabulated data and only reaches the Bethe tier when nothing tabulated resolves.🤖 Generated with Claude Code