Skip to content

fix: update material map validation script to Acts v46.8.1 - #1159

Open
wdconinc wants to merge 19 commits into
mainfrom
material-map-acts-46
Open

fix: update material map validation script to Acts v46.8.1#1159
wdconinc wants to merge 19 commits into
mainfrom
material-map-acts-46

Conversation

@wdconinc

@wdconinc wdconinc commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Dependencies

Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.

This PR updates the material map validation script to Acts v46.8.1:

  • Update ACTS_VERSION from v45.3.0 to v46.8.1 to match the Acts version in the platform
  • Remove PR #5359 patch application, as the fix landed in Acts v46.3.0 (function kept)
  • This fixes CI failures in the validate-material-map job after the Acts upgrade

What is the urgency of this PR?

  • High (please describe reason below)
  • Medium
  • Low

CI is currently broken on main.

What kind of change does this PR introduce?

Please check if any of the following apply

  • This PR introduces breaking changes. Please describe changes users need to make below.
  • This PR changes default behavior. Please describe changes below.
  • AI was used in preparing this PR. Please describe usage below.

Copilot AI lite review requested due to automatic review settings August 23, 2026 17:58
@github-actions github-actions Bot added the topic: infrastructure Regarding build system, CI, CD label Aug 23, 2026

This comment was marked as low quality.

veprbl
veprbl previously approved these changes Aug 24, 2026
@veprbl

veprbl commented Sep 1, 2026

Copy link
Copy Markdown
Member
Traceback (most recent call last):
  File "/home/runner/work/epic/epic/scripts/material_map/material_recording_epic.py", line 69, in <module>
    main()
    ~~~~^^
  File "/home/runner/work/epic/epic/scripts/material_map/material_recording_epic.py", line 59, in main
    runMaterialRecording(
    ~~~~~~~~~~~~~~~~~~~~^
        detector=detector,
        ^^^^^^^^^^^^^^^^^^
    ...<3 lines>...
        s=acts.examples.Sequencer(events=args.events, numThreads=1),
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ).run()
    ^
TypeError: runMaterialRecording() got an unexpected keyword argument 'outputDir'

@wdconinc

wdconinc commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author
TypeError: runMaterialRecording() got an unexpected keyword argument 'outputDir'

It's a different error at least now :-)

@wdconinc

wdconinc commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

It's a different error at least now :-)

It's all in the definition of "progress".

@wdconinc

wdconinc commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

FYI @ybedfer This is another change you will need, likely.

wdconinc and others added 10 commits September 1, 2026 16:56
- Update ACTS_VERSION from v45.3.0 to v46.8.1 to match the Acts version in the platform
- Remove PR #5359 patch application, as the fix landed in Acts v46.3.0 (function kept)
- This fixes CI failures in the validate-material-map job after the Acts upgrade

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The runMaterialRecording() function signature changed in Acts v46.8.1:
- Removed: outputDir parameter
- Added: outputFileBase parameter
- Moved: sequencer parameter to second position

This fixes the TypeError when running material map validation with Acts v46.8.1.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The Acts v46.8.1 APIs have significant changes:
- runMaterialValidation: Changed parameter order and replaced outputDir/outputName with outputFileBase
- runMaterialMapping: Changed to accept surfaces list and inputFile path instead of outputDir/inputDir

Both scripts now:
- Extract material surfaces from trackingGeometry using extractMaterialSurfaces()
- Pass surfaces as first parameter
- Use outputFileBase instead of separate directory/name parameters

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Changed while loop condition from '0 -gt 1' to '0 -gt 0' to properly
process single arguments. This fixes the --help flag not displaying help
when it's the only argument. Also added explicit -h|--help case handler.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- geometry_epic.py: Rename 'outputJson' → 'outputSurfacesJson' parameter
- material_mapping_epic.py: matFile defaults to 'material-map.json' (produces '_map.json' output)
- material_validation_epic.py: matFile now optional (empty string default)
- epic.py: Add file existence check before loading material decorator
- run_material_map_validation.sh:
  - Fix --help flag handling (was requiring 2+ arguments)
  - Use correct material-map filenames (material-map.json input → material-map_map.json output)
  - Pass correct generated material map file to validation step
  - Disable visualization scripts that fail on geometry JSON schema mismatch (temporary workaround)

Note: v46.8.1 removed CBOR support; material maps now output as JSON and ROOT formats only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
wdconinc and others added 2 commits September 1, 2026 19:03
The geo_id field in Acts v46.8.1 changed from a simple uint64_t to an object
with component fields (volume, boundary, layer, approach, sensitive).
This patch reconstructs the uint64_t by bitwise ORing the components according
to the GeometryIdentifier bit layout:
- volume: bits 56-63
- boundary: bits 48-55
- layer: bits 36-47
- approach: bits 28-35
- sensitive: bits 0-27

Handles both old (numeric) and new (object) geo_id formats for compatibility.
The patch is applied early in run_material_map_validation.sh before visualization
scripts execute, enabling all surface plots to work with v46.8.1 geometry output.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread scripts/material_map/materialPlotHelper.cpp.patch
Comment thread scripts/material_map/epic.py
@wdconinc

wdconinc commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

So, this runs now to completion, but doesn't produce the right material maps. I propose to merge as is, which unblocks some CI, and continue looking into the reasons why this fails. @veprbl @ShujieL

Current:
Ratio_Val_geant_mat_X0_Eta

Regenerated:
Ratio_Val_geant_mat_X0_Eta

@wdconinc
wdconinc requested review from a team and ShujieL September 3, 2026 18:19
@wdconinc
wdconinc enabled auto-merge (squash) September 3, 2026 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: infrastructure Regarding build system, CI, CD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants