Skip to content

Add A/E resolution term from electronic noise#53

Merged
tdixon97 merged 3 commits intolegend-exp:mainfrom
tdixon97:pulse_par_changes
Feb 6, 2026
Merged

Add A/E resolution term from electronic noise#53
tdixon97 merged 3 commits intolegend-exp:mainfrom
tdixon97:pulse_par_changes

Conversation

@tdixon97
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

❌ Patch coverage is 58.82353% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.26%. Comparing base (a8e6c1a) to head (fb3317c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...imflow/scripts/extract_hpge_current_pulse_model.py 0.00% 26 Missing ⚠️
workflow/src/legendsimflow/hpge_pars.py 78.94% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   63.05%   63.26%   +0.21%     
==========================================
  Files          19       19              
  Lines        1394     1481      +87     
==========================================
+ Hits          879      937      +58     
- Misses        515      544      +29     

☔ 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 pulse_par_changes branch 2 times, most recently from a602005 to 82c4507 Compare January 14, 2026 10:55
@gipert gipert marked this pull request as draft January 15, 2026 11:32
@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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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 12 out of 12 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (3)

workflow/src/legendsimflow/scripts/tier/evt.py:36

  • The evt.py script is incomplete. It only creates an empty file using Path(evt_file).touch() but doesn't actually process the opt_file and hit_file inputs to generate proper evt tier data. The input files (opt_file and hit_file) are defined but never used. This suggests the implementation is not finished.
    workflow/src/legendsimflow/scripts/tier/evt.py:3
  • Trailing comma after the author's name in the copyright line should be removed.
    workflow/src/legendsimflow/scripts/tier/evt.py:21
  • Import of 'legenddataflowscripts' is not used.

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

@gipert
Copy link
Member

gipert commented Feb 5, 2026

This needs a rebase

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

tdixon97 commented Feb 5, 2026

Yes needs rebase but also there is some issue to debug (lots of nans)

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Feb 5, 2026

actually ... seems the AI caught a bug that might be the issue

@tdixon97 tdixon97 marked this pull request as ready for review February 5, 2026 10:22
@tdixon97
Copy link
Collaborator Author

tdixon97 commented Feb 5, 2026

This is now working well, but we have to decide how to save the plots...

Screenshot from 2026-02-05 13-05-29

@tdixon97 tdixon97 force-pushed the pulse_par_changes branch 2 times, most recently from 9d1b53d to 68af953 Compare February 5, 2026 19:04
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 6 out of 6 changed files in this pull request and generated 12 comments.

Comments suppressed due to low confidence (1)

workflow/src/legendsimflow/hpge_pars.py:536

  • The df_cfg variable is assigned on lines 522-526 based on whether 'setups' exists in dataflow_config, but then it's immediately overwritten on line 536 with dataflow_config.paths. This makes the conditional assignment on lines 522-526 pointless. This appears to be dead code that should either be removed or the logic needs to be corrected to use the conditional df_cfg assignment consistently.
    df_cfg = (
        dataflow_config["setups"]["l200"]["paths"]
        if ("setups" in dataflow_config)
        else dataflow_config["paths"]
    )

    # get the reference cal run
    cal_runid = mutils.reference_cal_run(metadata, runid)
    _, period, run, _ = re.split(r"\W+", cal_runid)

    msg = f"inferred reference calibration run: {cal_runid}"
    log.debug(msg)

    # get the paths to hit and raw tier files
    df_cfg = dataflow_config.paths

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

@gipert
Copy link
Member

gipert commented Feb 5, 2026

can you fix the docs?

@gipert gipert changed the title add the current_reso Add A/E resolution term from electronic noise Feb 5, 2026
@gipert gipert linked an issue Feb 5, 2026 that may be closed by this pull request
@tdixon97 tdixon97 merged commit 6513a39 into legend-exp:main Feb 6, 2026
7 of 8 checks passed
@gipert
Copy link
Member

gipert commented Feb 6, 2026

why did you merge this if the CI is failing

@tdixon97
Copy link
Collaborator Author

tdixon97 commented Feb 6, 2026

Check the CI failure, its not our fault

@gipert
Copy link
Member

gipert commented Feb 6, 2026

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.

A max resolution parameters

2 participants