Skip to content

function for lar inside / outside NMS (l200)#21

Merged
gipert merged 15 commits intolegend-exp:mainfrom
tdixon97:lar_split
Feb 12, 2026
Merged

function for lar inside / outside NMS (l200)#21
gipert merged 15 commits intolegend-exp:mainfrom
tdixon97:lar_split

Conversation

@tdixon97
Copy link
Collaborator

Not clear where this belongs.

@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

❌ Patch coverage is 84.44444% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.38%. Comparing base (b18aae0) to head (e473d36).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
workflow/src/legendsimflow/commands.py 76.19% 10 Missing ⚠️
workflow/src/legendsimflow/confine.py 91.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   61.14%   62.38%   +1.24%     
==========================================
  Files          20       21       +1     
  Lines        1588     1675      +87     
==========================================
+ Hits          971     1045      +74     
- Misses        617      630      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tdixon97 tdixon97 force-pushed the main branch 3 times, most recently from 7fa7fa8 to 189d6c5 Compare November 24, 2025 22:27
@tdixon97 tdixon97 force-pushed the main branch 2 times, most recently from ae04198 to 67177fd Compare December 19, 2025 14:39
@gipert gipert marked this pull request as draft January 15, 2026 11:33
@tdixon97 tdixon97 marked this pull request as ready for review February 2, 2026 19:15
Copilot AI review requested due to automatic review settings February 2, 2026 19:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds utility functions to generate LAr confinement commands for remage simulations, specifically for confining particle generation inside or outside the minishroud (NMS) regions in the L200 geometry. The functions parse GDML geometry using pyg4ometry and output configuration commands.

Changes:

  • Added _get_matching_volumes helper function to match volume names using wildcard patterns
  • Added get_lar_minishroud_confine_commands function to extract geometry parameters and generate remage confinement commands
  • Added pyg4ometry import for GDML parsing

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@gipert gipert marked this pull request as draft February 5, 2026 10:07
@tdixon97
Copy link
Collaborator Author

I added the option to run in the simflow based on the new confinment option "~lar_volumes" which can be either "inside_nms" or "outside_nms".

In the end it would been nice to have a flexible interace but it ends up a bit challenging, I could think of nothing not involving evaling strings. I also did not particularly want to make a new tier, although this is of course an option (handled similarly to vtx).

I think this is ugly but I do not have a better suggestion

@tdixon97
Copy link
Collaborator Author

Screenshot from 2026-02-10 00-55-52 Basically works but there are a few oversights.
  • Outside nms uses full LAr volume
  • complicated to have it be more specific eg. just one string etc without introducing a whole language.

@tdixon97 tdixon97 force-pushed the lar_split branch 3 times, most recently from 7d4c14f to c666099 Compare February 10, 2026 01:00
@tdixon97
Copy link
Collaborator Author

Screenshot from 2026-02-10 01-06-15 Sort of works for outside but is also just the full cryostat

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 8 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@gipert
Copy link
Member

gipert commented Feb 10, 2026

why don't we implement a very simple "processor spec" where some args like the registry are always provided internally? similarly to what we do in the pygama evt tier:

https://github.com/legend-exp/pygama/blob/a8bea112ecfa1e0640f3f313f4398bf283b068f8/src/pygama/evt/build_evt.py#L510-L561

but simpler. the syntax could be something like

confinement: ~function:do_the_thing(..., is_inside=True)

where ... is substituted with the implicit args (see pygama code). i would put all these special functions in a dedicated module and that is imported so we can skip the auto import logic.

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Feb 10, 2026 via email

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Feb 10, 2026 via email

tdixon97 and others added 4 commits February 11, 2026 16:45
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tdixon97 and others added 3 commits February 11, 2026 16:45
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@tdixon97 tdixon97 marked this pull request as ready for review February 11, 2026 17:18
@tdixon97 tdixon97 requested a review from Copilot February 11, 2026 17:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (1)

workflow/src/legendsimflow/commands.py:432

  • This error message lists ~define: but the implemented prefix is ~defines:. Since this string is shown to users on config errors, please correct it (and keep the list of valid prefixes consistent with the actual startswith checks).
            msg = (
                (
                    "the field must be a str or list[str] prefixed by "
                    "~define: / ~volumes.surface: / ~volumes.bulk: / ~function: or ~vertices:"
                ),
                f"{block}.confinement",
            )
            raise SimflowConfigError(*msg)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tdixon97 tdixon97 marked this pull request as draft February 11, 2026 23:27
@tdixon97
Copy link
Collaborator Author

@gipert if you add the better handling of the geometry, I think this is ready (apart from the detail of what outer LAr volume to use). Then we have everything for the stp tier...

@tdixon97 tdixon97 marked this pull request as ready for review February 12, 2026 13:38
@gipert gipert merged commit 178ff24 into legend-exp:main Feb 12, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants