Skip to content

Add julia script for ideal Pulse Shape Library generation#57

Merged
gipert merged 1 commit intolegend-exp:mainfrom
SalehGiovanna:pulse-shape-modeling
Mar 20, 2026
Merged

Add julia script for ideal Pulse Shape Library generation#57
gipert merged 1 commit intolegend-exp:mainfrom
SalehGiovanna:pulse-shape-modeling

Conversation

@SalehGiovanna
Copy link
Copy Markdown

@SalehGiovanna SalehGiovanna commented Jan 19, 2026

Julia script to build ideal Pulse Shape Library using ssd

Realistic PSL construction from ideal (py) ---> Moved to separate PR

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.96%. Comparing base (d7a810d) to head (88133c7).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   67.13%   67.96%   +0.82%     
==========================================
  Files          23       23              
  Lines        2054     2060       +6     
==========================================
+ Hits         1379     1400      +21     
+ Misses        675      660      -15     

☔ 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.

Comment thread output Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
@tdixon97
Copy link
Copy Markdown
Collaborator

Can you run pre-commit locally, this will include checks on the julia code and formatting

Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
@tdixon97 tdixon97 marked this pull request as ready for review January 27, 2026 17:08
@tdixon97 tdixon97 marked this pull request as draft January 27, 2026 17:08
@tdixon97 tdixon97 marked this pull request as ready for review January 27, 2026 17:09
@SalehGiovanna SalehGiovanna changed the title Add scripts for HPGe pulse shape modeling Add scripts for HPGe pulse shape modeling (PSL) Jan 29, 2026
Copilot AI review requested due to automatic review settings January 29, 2026 16:30
Copy link
Copy Markdown
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 pull request adds two new scripts for HPGe pulse shape modeling to create a Pulse Shape Library (PSL). The scripts work together to generate realistic waveform maps from ideal simulated waveforms.

Changes:

  • Added make_hpge_ideal_wf_map.jl to generate ideal waveform maps from SolidStateDetectors simulations
  • Added make_hpge_realistic_wf_map.py to transform ideal waveform maps into realistic ones through convolution with electronics response

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 29 comments.

File Description
workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Julia script that simulates ideal charge waveforms for HPGe detectors at different crystal axis angles using SolidStateDetectors, normalizes them, and saves to LH5 format
workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Python script that processes ideal waveform maps by convolving with system response kernels, converting to current waveforms, applying moving window averaging, and aligning peaks to create realistic PSL compatible with reboost

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

Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_realistic_wf_map.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Copy link
Copy Markdown
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 3 out of 3 changed files in this pull request and generated 19 comments.


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

Comment thread workflow/src/legendsimflow/psl.py Outdated
Comment thread workflow/src/legendsimflow/psl.py Outdated
Comment thread workflow/src/legendsimflow/psl.py Outdated
Comment thread workflow/src/legendsimflow/psl.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/psl.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/psl.py Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
@gipert
Copy link
Copy Markdown
Member

gipert commented Feb 5, 2026

see #91

@SalehGiovanna
Copy link
Copy Markdown
Author

@gipert I saw it, but I am not using those functions in my script, even though its true that there is a lot of shared code.
So the possibilities I see are:

@gipert
Copy link
Copy Markdown
Member

gipert commented Feb 6, 2026

Yes, the first is the right solution

@SalehGiovanna SalehGiovanna changed the title Add scripts for HPGe pulse shape modeling (PSL) Add julia script for ideal Pulse Shape Library generation Feb 19, 2026
@tdixon97 tdixon97 requested a review from Copilot February 20, 2026 14:06
Copy link
Copy Markdown
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 15 comments.


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

Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
@tdixon97
Copy link
Copy Markdown
Collaborator

tdixon97 commented Mar 18, 2026

@SalehGiovanna I rebased this onto #95. I think its in a great shape, I can quickly make the last fixes then we are done with this (we can add tests in a further PR)

Copy link
Copy Markdown
Collaborator

@tdixon97 tdixon97 left a comment

Choose a reason for hiding this comment

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

Just some small comments to remove the outdated options, this would also align with the dt map script

Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
@gipert
Copy link
Copy Markdown
Member

gipert commented Mar 19, 2026

after #95 got merge the situation got messy, i work on a rebase now

@gipert gipert force-pushed the pulse-shape-modeling branch 2 times, most recently from e1d302a to 3e4c560 Compare March 19, 2026 10:11
@tdixon97
Copy link
Copy Markdown
Collaborator

tdixon97 commented Mar 19, 2026

@gipert now the commits are just by you and Claude, do you know how to fix this to be Giovanna?
I suppose its due to rebasing?
By the way @SalehGiovanna and I discussed a refactoring to resolve almost all comments, she will implement it today I think

@SalehGiovanna
Copy link
Copy Markdown
Author

@tdixon97 yes i'm doing it now, I did not commit anything yet because I saw there was ongoing work, now i do it

@gipert gipert force-pushed the pulse-shape-modeling branch 2 times, most recently from 670be1f to a118787 Compare March 19, 2026 11:13
@gipert
Copy link
Copy Markdown
Member

gipert commented Mar 19, 2026

I fixed the authors. I just have a chat with @SalehGiovanna, she is gonna pick the work up from the current status here.

@tdixon97
Copy link
Copy Markdown
Collaborator

Now the authors is me and you Luigi? Should be Giovanna

@gipert
Copy link
Copy Markdown
Member

gipert commented Mar 19, 2026

The first commit is giovanna's work and she's an author there

Copy link
Copy Markdown
Collaborator

@hervasa2 hervasa2 left a comment

Choose a reason for hiding this comment

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

Very nice! Please see minor requested changes and comments from the SSD side

Comment thread workflow/src/LegendSimflow.jl/src/drift_time_helpers.jl Outdated
Comment thread workflow/src/LegendSimflow.jl/src/drift_time_helpers.jl Outdated
Comment thread workflow/src/LegendSimflow.jl/src/drift_time_helpers.jl Outdated
Comment thread workflow/src/LegendSimflow.jl/src/pulse_shape_sim_helpers.jl
Comment thread workflow/src/LegendSimflow.jl/src/pulse_shape_sim_helpers.jl Outdated
Comment thread workflow/src/legendsimflow/scripts/make_hpge_ideal_wf_map.jl Outdated
@gipert gipert mentioned this pull request Mar 19, 2026
@hervasa2
Copy link
Copy Markdown
Collaborator

I would move to merge this asap and then to switch to the new ssd on everything on a subsequent PR

@tdixon97
Copy link
Copy Markdown
Collaborator

tdixon97 commented Mar 19, 2026

We first need to remove the duplicate code and add (atleast basic) tests. For example even a test the code runs is valuable

@gipert
Copy link
Copy Markdown
Member

gipert commented Mar 19, 2026

important missing thing: extend the PSL outside its bounds as done at the end of the function that makes the dt map

@SalehGiovanna
Copy link
Copy Markdown
Author

But then how do we handle those waveforms?

@tdixon97
Copy link
Copy Markdown
Collaborator

I added a few (basic) tests, that the ssd code runs

@hervasa2
Copy link
Copy Markdown
Collaborator

But then how do we handle those waveforms?

You don's simulate those waveforms. You just copy the waveform from the nearest pixel inside the detector, just like the dt map (but with a waveform instead of the dt)

@SalehGiovanna
Copy link
Copy Markdown
Author

But isn't it better to have waveforms outside the detector volume being jus NaNs, so they are easy to identify later?

@gipert
Copy link
Copy Markdown
Member

gipert commented Mar 20, 2026

But isn't it better to have waveforms outside the detector volume being jus NaNs, so they are easy to identify later?

no, the pixelation will always leave some holes in valid areas of the detector, so we want to extend the domain to be safe

@gipert gipert marked this pull request as ready for review March 20, 2026 13:22
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Giovanna Saleh <giovanna.saleh@phd.unipd.it>
Co-Authored-By: Toby Dixon <toby.dixon.23@ucl.ac.uk>
@gipert gipert force-pushed the pulse-shape-modeling branch from 162fc9e to 88133c7 Compare March 20, 2026 13:41
@gipert gipert merged commit bf9fa0c into legend-exp:main Mar 20, 2026
10 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.

5 participants