Beam Caching by bhardwaj-gopika · Pull Request #65 · slaclab/virtual-accelerator

bhardwaj-gopika · 2026-09-15T17:59:46Z

Adds infrastructure for storing reference beam distributions at handoff planes under virtual_accelerator/beams/, backed by Git LFS.

Enforcement checks:

Pytest test (virtual_accelerator/tests/test_beam_sidecars.py) — parametrized over every .h5 under beams/. Asserts each has a matching .json sidecar containing all required fields. Runs in CI, so PRs cannot merge with missing or malformed sidecars.
Pre-commit hook (beam-h5-has-sidecar in .pre-commit-config.yaml, powered by scripts/check_beam_sidecars.py) — fires on staged .h5 files under beams/ and blocks the commit if any lack a sidecar.

Both operate on filenames, so LFS storage vs plain-git storage of the .h5 is irrelevant to the check.

…responding json

Copilot

🟡 Changes recommended

Moderate issues remain in sidecar enforcement, documented metadata validation, and default-beam integration coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds Git LFS-backed FACET2 beam distributions with JSON sidecars, validation checks, CI support, and related model API updates.

Changes:

Adds cached beam datasets and metadata.
Adds pytest and pre-commit sidecar enforcement.
Updates FACET2 defaults, scalar access, documentation, and CI LFS checkout.

File summaries

File	Summary
`virtual_accelerator/tests/test_surrogates.py`	Updates scalar reads.
`virtual_accelerator/tests/test_static_model.py`	Updates scalar reads.
`virtual_accelerator/tests/test_staged_model.py`	Updates scalar reads.
`virtual_accelerator/tests/test_facet.py`	Updates scalar reads.
`virtual_accelerator/tests/test_cu_hxr.py`	Updates scalar reads.
`virtual_accelerator/tests/test_beam_sidecars.py`	Validates beam sidecars; does not enforce all documented required fields (moderate, 2 votes).
`virtual_accelerator/tests/_bmad_model_test_utils.py`	Updates model helper reads.
`virtual_accelerator/models/facet2.py`	Uses the cached default beam and new scalar API; default-beam integration coverage is missing (moderate, 1 vote).
`virtual_accelerator/beams/2024-10-22_facet2_oneBunch/PR10241_100000.json`	Adds beam metadata.
`virtual_accelerator/beams/2024-10-22_facet2_oneBunch/PR10241_100000.h5`	Adds an LFS beam distribution.
`virtual_accelerator/beams/2024-10-22_facet2_oneBunch/PR10241_10000.json`	Adds beam metadata.
`virtual_accelerator/beams/2024-10-22_facet2_oneBunch/PR10241_10000.h5`	Adds an LFS beam distribution.
`virtual_accelerator/beams/2024-10-22_facet2_oneBunch/L0AFEND_100000.json`	Adds handoff beam metadata.
`virtual_accelerator/beams/2024-10-22_facet2_oneBunch/L0AFEND_100000.h5`	Adds the default LFS beam distribution.
`scripts/check_beam_sidecars.py`	Implements sidecar checking but inspects only the working tree rather than the Git index (moderate, 3 votes).
`README.md`	Documents beam storage and sidecar metadata requirements.
`.pre-commit-config.yaml`	Registers the sidecar hook, but its path filter can bypass sidecar deletion or renaming (moderate, 1 vote).
`.gitignore`	Allows beam HDF5 files.
`.github/workflows/tests.yml`	Enables Git LFS checkout.
`.github/workflows/test-core.yml`	Enables Git LFS checkout.
`.github/workflows/test-bmad.yml`	Enables Git LFS checkout.
`.gitattributes`	Configures HDF5 files for Git LFS.

Review details

Suppressed comments (2)

.pre-commit-config.yaml:34

This path filter only runs the hook when an .h5 pathname is staged. Staging deletion or renaming of foo.json while leaving foo.h5 unchanged therefore bypasses the hook and can commit a beam with no sidecar. Include sidecar changes in the trigger and normalize those inputs to their corresponding .h5 file, or have the hook scan all affected beam pairs.

        files: ^virtual_accelerator/beams/.*\.h5$

virtual_accelerator/models/facet2.py:170

This new default is used only when track_beam=True, start_element="L0AFEND", and no custom_beam_path is supplied, but the FACET tracking tests always pass TEST_BEAM_PATH (for example virtual_accelerator/tests/test_facet.py:60-94). A bad path or incompatible LFS beam can therefore reach users without CI detecting it; add an integration test that exercises the default-beam branch.

        default_beam_relpath="../beams/2024-10-22_facet2_oneBunch/L0AFEND_100000.h5",

Files reviewed: 21/25 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…PMD.

roussel-ryan

Looks pretty good. A couple of comments,

we will need a top level API similar to the model registry API to get the particle beams easily, potentially with some kind of search function to look for beams at given locations and for certain machines
we should only have a single, large particle distribution for each location / time / operating mode, downsampling should be done by ParticleGroup.resample
can we just do .meta.json instead of h5.meta.json for the filenames? If there is a good reason to keep it as is then I'm willing to accept it, otherwise it would be good to key things just by the base file name

pluflou · 2026-09-28T19:29:02Z

+  "date_generated": "2024-10-22",
+  "mode": "nominal_one_bunch",
+  "source": "FACET2-S2E 2024-10-22 one-bunch scenario; exact upstream file not confirmed",
+  "notes": "Default beam for the FACET-II Bmad model (resolved by virtual_accelerator.beams.get_beam). Mean z and energy computed with pmd_beamphysics.ParticleGroup: z=4.128 m, energy=68.62 MeV. Generator unclear from filename; the FACET2-S2E 2024-10-22_Impact_OneBunch run stops at PR10241 upstream of L0AF, so this file is likely a downstream track (Bmad or otherwise), not the raw IMPACT output."


does this note need updating since you checked the source?

pluflou · 2026-09-28T19:43:35Z

+    element: str,
+    mode: str,
+):
+    """Return every cached beam matching ``(beamline, element, mode)``.


can we add full dosctrings for all user facing methods/functions?

pluflou · 2026-09-30T21:47:34Z

        "TCY10490": "KLYS:LI10:51",
    }

+    if track_beam and custom_beam_path is None and start_element == "L0AFEND":


@roussel-ryan do we also need to update the logic in the bmad factory or are those beams handled differently?

yes we will need to update the logic in the factory methods, that can come in a later PR

We should remove the changes to this file in the meantime

pluflou

Looks good overall. I just would suggest full/clear docstrings for all user-facing methods/functions.

roussel-ryan · 2026-10-02T18:59:35Z

this file and its corresponding sidecar should be removed in favor of the larger particle file

…larger particle file

bhardwaj-gopika added 4 commits September 15, 2026 10:58

Beam Caching

077d10a

Fixing tests

39bee38

Adding precommit hooks and tests to make sure all h5 files have a cor…

75faaeb

…responding json

linting

e3a886c

bhardwaj-gopika marked this pull request as ready for review September 16, 2026 16:36

bhardwaj-gopika requested review from pluflou and roussel-ryan and a lite review from Copilot September 16, 2026 16:36

Copilot started reviewing on behalf of bhardwaj-gopika September 16, 2026 16:37 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread scripts/check_beam_sidecars.py Outdated

Comment thread virtual_accelerator/tests/test_beam_sidecars.py

bhardwaj-gopika added 7 commits September 17, 2026 16:47

impact_inj_50k_YAG03.h5

4b28bae

ensure H5 files can be loaded into a particle group object using Open…

0402079

…PMD.

Renaming json to reflect metadata

e4d34ce

Syncing up branch

72d99e9

fixing linting

c396e3d

adding impact source

e733d3e

Addressing comments

6e8f883

roussel-ryan reviewed Sep 22, 2026

View reviewed changes

bhardwaj-gopika added 2 commits September 25, 2026 10:07

beam registry

df8f9eb

syncing with main

096f5a0

pluflou reviewed Sep 28, 2026

View reviewed changes

pluflou reviewed Sep 30, 2026

View reviewed changes

bhardwaj-gopika added 2 commits September 30, 2026 15:52

Addressing comments

0440dda

Resolving merge conflicts and ruff issues

ee077d7

roussel-ryan reviewed Oct 2, 2026

View reviewed changes

bhardwaj-gopika added 2 commits October 2, 2026 14:10

file and its corresponding sidecar should be removed in favor of the …

d7b953a

…larger particle file

Syncing w main

5b2bf69

Navigation Menu

Beam Caching - #65

Beam Caching#65
bhardwaj-gopika wants to merge 17 commits into
mainfrom
beam_caching

bhardwaj-gopika commented Sep 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

roussel-ryan left a comment

Uh oh!

pluflou Sep 28, 2026

Uh oh!

pluflou Sep 28, 2026

Uh oh!

pluflou Sep 30, 2026

Uh oh!

roussel-ryan Oct 2, 2026

Uh oh!

roussel-ryan Oct 2, 2026

Uh oh!

pluflou left a comment

Uh oh!

roussel-ryan Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bhardwaj-gopika commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

roussel-ryan left a comment

Choose a reason for hiding this comment

Uh oh!

pluflou Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

pluflou Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

pluflou Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

roussel-ryan Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

roussel-ryan Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

pluflou left a comment

Choose a reason for hiding this comment

Uh oh!

roussel-ryan Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bhardwaj-gopika commented Sep 15, 2026 •

edited

Loading