Skip to content

Use the new sigproc binner#32

Closed
kylmcgr wants to merge 3 commits into
mainfrom
use-sigproc-binner
Closed

Use the new sigproc binner#32
kylmcgr wants to merge 3 commits into
mainfrom
use-sigproc-binner

Conversation

@kylmcgr

@kylmcgr kylmcgr commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Uses the new sigproc binner. When we merge the sigproc PR, I can take out the pyproject toml line.

@kylmcgr kylmcgr requested a review from cboulay June 25, 2026 23:35
@kylmcgr kylmcgr self-assigned this Jun 25, 2026
cboulay added a commit that referenced this pull request Jun 26, 2026
binned.py still carried its own integer-locked event-counting binner, the last
event binner not on the shared grid. Rewrite it as a thin wrapper over
ezmsg.sigproc.binned_aggregate.BinnedAggregate (operation=SUM): densify sparse
events to per-sample contributions, delegate all boundary/carry/axis math to
the shared binner, and optionally rate-normalize.

Ported from #32 (Kyle McGraw), adjusted for the enhanced sigproc shipped in
2.26.0:
- fractional=False is documented and tested as int(bin_duration*fs) truncation
  (matching Window and BinSchedule), not round(); the sample-locked test now
  includes fs=30030 where truncation (600) and rounding (601) diverge.
- Adds scale_by_value (weight events by stored value), a binned_event_aggregator
  factory, and tests for the off-nominal fractional grid, chunk invariance,
  count-vs-rate scaling, scale_by_value, and the sample-locked grid.

This leaves Rate/EventRate on the kernel_activation path (event-optimized,
O(n_events) sparse) from the companion commits; BinnedEventAggregator is now
also on the shared grid, so no event binner remains on the old divergent one.
@cboulay

cboulay commented Jun 26, 2026

Copy link
Copy Markdown
Member

Thanks @kylmcgr — the binned.py modernization here has been carried into #33 (commit 5b69c92) so it merges cleanly on top of the enhanced sigproc.

Two things to note about how it was brought over:

  • Adjusted for sigproc 2.26.0's int() truncation. This PR was based on the original generalize-binner (which used round(bin_duration*fs) for sample-locked mode). The released 2.26.0 changed that to int() truncation so the sample-locked grid matches Window exactly. The ported docstrings/tests reflect that, and test_fractional_false_sample_locked gained an fs=30030 case (0.02·fs = 600.6) — the discriminating point where truncation (600) and rounding (601) diverge, which the 30000/30012 cases here couldn't catch.
  • Rate/EventRate kept on the kernel_activation path rather than re-pointed through BinnedEventAggregator. That preserves the O(n_events) sparse path for spike data (no full densify per chunk) and the kernel family, while still sharing the grid via BinSchedule. BinnedEventAggregator itself is now also on the shared binner, so no event binner is left on the old divergent grid.

Everything green against released 2.26.0 (full suite 147 passed). I think this PR can be closed in favor of #33 — but flagging here so the attribution and the roundint delta are on the record first. Let me know if you'd prefer to keep it open for any reason.

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