Skip to content

Convert VectorOfVectors outputs in the first block too - #227

Closed
ggmarshall wants to merge 1 commit into
legend-exp:mainfrom
ggmarshall:fix-vov-output-unit-conversion
Closed

ggmarshall wants to merge 1 commit into
legend-exp:mainfrom
ggmarshall:fix-vov-output-unit-conversion

Conversation

@ggmarshall

@ggmarshall ggmarshall commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor
  • I have reviewed all the code in this pull request and deem it of the standard expected for this project.

Problem

Since #213, LGDOVectorOfVectorsIOManager creates its buffer lazily in the first write(). If the output's unit differs from the variable's (e.g. a timepoint found in sample index and written in ns), get_buffer adds a UnitConversionManager to the chain at that point. By then the first block's processors have already run, so the first block is written from an unfilled buffer.

Result: the first block_width entries of every chain come out as 0, while the vector lengths are still correct. In LEGEND's SiPM chain this hits trigger_pos and trigger_pos_dplms. 2.2.0 got the buffer in __init__ and is not affected.

Fix

In write(), after the lazy get_buffer, run any processor managers it appended, so the block in flight is converted too. Later blocks are unchanged: the conversion is already in _proc_managers.

Tests

  • New test_vov_output_unit_conversion_first_block: 10 entries with block_width=4 and tp_max in ns. It fails on main (first 4 entries are 0) and passes with the fix.
  • Full suite passes locally: 104 tests.
  • I also ran 300 real LEGEND p18 SiPM traces through the SiPM chain with this fix. trigger_pos and trigger_pos_dplms match dspeed 2.2.0 output exactly; without the fix, 15 and 7 traces differ.

This PR was prepared with AI assistance (Claude Code) and reviewed by me.

🤖 Generated with Claude Code

LGDOVectorOfVectorsIOManager now creates its buffer lazily, in the first
write(). When the output unit differs from the variable's (e.g. sample
index -> ns), get_buffer appends a UnitConversionManager to the chain at
that point, after the block's processors have already run, so the first
block is written from an unfilled buffer (zeros). Run any conversion
get_buffer added for the block in flight.

Assisted-by: Generative AI
Copilot AI balanced review requested due to automatic review settings October 5, 2026 20:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Copilot review overview

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Adds a regression test and a processing-chain fix to ensure VectorOfVectors (VoV) outputs are unit-converted for the first processed block as well as subsequent blocks.

Changes:

  • Add a test that reproduces missing unit conversion for the first VoV block output.
  • Ensure any unit-conversion processors appended by get_buffer() are executed for the first block.
File Description
tests/​test_processing_chain.py Adds a regression test validating first-block VoV unit conversion.
src/​dspeed/​processing_chain.py Executes conversion processors appended by get_buffer() so the first block is converted.

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

Comment thread src/dspeed/processing_chain.py Resolved
Comment thread tests/test_processing_chain.py Resolved
@codecov

codecov Bot commented Oct 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.13%. Comparing base (ae7b4e1) to head (72ad10e).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   69.07%   69.13%   +0.05%     
==========================================
  Files          62       62              
  Lines        4188     4192       +4     
==========================================
+ Hits         2893     2898       +5     
+ Misses       1295     1294       -1     

☔ View full report in Codecov by Harness.
📢 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.

@iguinn

iguinn commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Better solution to bug in #228

@iguinn iguinn closed this Oct 5, 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.

3 participants