Add real-time multitaper spectrum diagnostics - #3
Open
galenlynch wants to merge 60 commits into
Open
galenlynch wants to merge 60 commits into
galenlynch wants to merge 60 commits into
Conversation
Track active acquisition callbacks before they access FIFO storage, reject starts that would replace a live worker, and retain processing state when a cooperative stop times out. Join the configuration builder cooperatively so JUCE never force-kills it while it owns planning state.
The display reducer previously created columns narrower than one FFT bin when a selected band contained fewer bins than horizontal pixels. Repeated values then rendered as artificial stair steps. Publish each native bin once when no downsampling is required. Continue using area-weighted means and peak envelopes when the spectrum has more bins than the display can represent.
A requested route can update editor state while the previous analysis remains live. The canvas previously refreshed channel labels only when its generation changed, so the legend could disagree with the frame being drawn. Refresh the bounded stream and channel metadata from every consumed frame. The legend now follows the data through channel and stream hot-swaps.
The growing control set was laid out as one vertical column and extended far below the host's fixed-height processor editor. Arrange input, display, amplitude, capture, and comparison controls in four compact columns. All controls now remain inside the signal-chain strip without removing diagnostic features.
Add optional aperiodic-background display and removal, stable automatic dB ranges, compact canvas controls, and a persistent peak-envelope toggle. Keep hidden peak data in the range fit so narrow spectral lines remain visible. Size transport slots for actual stream callback payloads instead of GenericProcessor's nominal 128-sample quantum, allowing analysis to start on the first acquisition. Cover the DSP, transport, lifecycle, and rendering behavior with regressions.
The acquisition, analysis, configuration, and message threads exchange several generation-tagged objects. Document their ownership, mailbox and FIFO boundaries, configuration replacement sequence, overload behavior, and shutdown ordering so future changes preserve the real-time contract.
Building OpenBLAS during plugin configuration made setup slow and tied the numerical backend to each local toolchain. Fetch pinned conda-forge packages into a build-local, content-addressed stage, with a prepared-root override for offline builds. Preserve package provenance and notices, hide the static symbols on Unix, and namespace the Windows runtime. Limit OpenBLAS to one thread before the first eigensolve so asynchronous DPSS preparation cannot oversubscribe the host.
Spectrum Viewer CI repeatedly failed because it built against the main FFTW branch, which lacks the required batched API. Windows also selected a runner without the requested Visual Studio 2022 generator. Build the stacked FFTW branch on stable native runners, test the DSP suite, and validate private OpenBLAS linkage. Upload short-lived review artifacts, including a self-contained Windows rig-test bundle, while retaining JFrog deployment for main only.
The managed FFTW bundle uses lib-prefixed filenames on Unix, but Spectrum Viewer clears CMake's default shared-library prefix. Its tests consequently failed to find either precision in the new staging directories. Search the managed layouts explicitly and pass a platform-neutral relative checkout path. This also prevents Windows from interpreting the backslashes in github.workspace as part of the CMake cache value.
galenlynch
changed the title
Collaborator
Author
|
Depends on open-ephys-plugins/OpenEphysFFTW#4 |
The managed FFTW link libraries name versioned runtime files that live in the bundle's bin directory. The test process could therefore link successfully but failed before main when the system did not provide the same FFTW version. Resolve both staged runtimes for every supported platform, copy them beside the test executable, and use an executable-relative build rpath on Unix. Tests now exercise the dependency bundle instead of an incidental system installation.
The optimized trace renderer inherited from XYLine and called its methods directly. The supported Windows host import library does not export those symbols, so the plugin could not link even though InteractivePlot itself is part of the plugin API. Store line data in FrequencyPlot and draw each trace as one JUCE path from paintOverChildren. This preserves the measured batching improvement without adding an undeclared host ABI requirement.
JUCE imports Cocoa from its core implementation on macOS, so compile that translation unit as Objective-C++ and link its declared system frameworks. Use runtime-sized pointer storage in the callback-partition test because MSVC does not accept the enclosing function's constant as a template argument inside the lambda.
The Windows test objects imported JUCE symbols even though JuceCore.cpp compiled those symbols into the test executable. This mismatch left AbstractFifo and other core methods unresolved at link time.\n\nForce a test-only JUCE configuration header into every Windows translation unit so the standalone executable consistently owns JUCE core.
The Windows test executable did not link. Tests/CMakeLists.txt defines OEPLUGIN at directory scope, so the GUI's CommonLibHeader.h resolved COMMON_LIB to __declspec(dllimport) for the OpenEphysFFTW batch classes that the test binary compiles into itself, and it pulled JuceHeader.h into translation units that only need juce_core. juce_Colours.h defines namespace-scope Colour constants, so every such unit emitted a reference to Colour::Colour(uint32) -- a juce_graphics symbol that a headless test binary does not compile and, since ad8ff57 blanked JUCE_API, no longer imported either. Shadow CommonLibHeader.h with a test-only stub that defines COMMON_LIB as empty and includes no JUCE. The tests need the FFTW batch declarations, not the visualizer surface. This also clears the LNK4217/LNK4286 warnings about importing symbols defined in the same binary.
Eight defects found while auditing the branch for production readiness. No behavioural change when everything is well-formed. Crashes: - SpectrumViewerEditor::selectedStreamHasChanged dereferenced getDataStream(getCurrentStream()) guarded only by a nonempty stream list. A nonempty list does not mean the cached selection still names a live stream, and live stream replacement makes that churn likely. - CanvasPlot::paint indexed chanColors with .at(). An exception escaping a JUCE paint call terminates the host. The plot loop indexed several parallel arrays on an assumed bound; make the bound explicit. Routing: - parameterValueChanged accepted any stream's "Channels" parameter. It is stream-scoped, so every stream owns one and every one of them reports here; during acquisition this routed another stream's channel list to the active runtime. Session loading delivers them in arbitrary order. Termination paths: - AsyncSpectrumAnalysis's constructor threw if its thread failed to start. It is a direct member of SpectrumViewer, so the throw escaped Plugin::createProcessor across the plugin ABI. Degrade instead and let request() report the failure through the existing result path, which the worker already handles as configurationFailed. - retire() could throw bad_alloc from ~SpectrumViewer and from inside Thread::run(). Make it noexcept with a reserved vector, falling back to destroying on the calling thread rather than terminating. Memory ordering: - stopAcquisition release-stored acquisitionRunning then acquire-loaded activeAudioCallbacks, while process() did the mirror pair. Store-load is not ordered, so both sides could miss each other and clearAcquisitionState could free the input FIFO under a live callback. Both pairs are now seq_cst, which is what the comment already claimed. - Both seqlock readers loaded the payload relaxed and re-read the sequence with acquire. Acquire constrains later accesses, not earlier ones, so it did not stop the payload loads sinking below the re-read. Add the acquire fence. The input route payload is eleven words wide. - readDisplaySettings spun unbounded on an odd sequence while CanvasPlot::resized publishes an update per resize event. Bound it like readInputRoute, and skip publishing a frame rather than reducing one against a torn snapshot.
Replaces LAPACKE_dstevr for DPSS generation with a plugin-local Sturm
bisection plus inverse iteration, written from the published algorithms
(Barth/Martin/Wilkinson 1967; Peters/Wilkinson 1979; Golub & Van Loan
8.2 and 8.4) rather than transliterated from LAPACK. The public entry
point now routes to it; the LAPACK path is retained one more commit so
the parity tests can compare them.
The spectral estimation algorithm is unchanged. Only the eigen-backend
moves, and DpssTapers reads just the eigenvectors -- concentration ratios
come from its own FFT, not from the eigenvalues.
Measured on the Slepian matrices the plugin actually builds:
N=15000 K=8 lapack 55.3 ms in-tree 36.6 ms ratio 0.66
N=60000 K=5 lapack 139.8 ms in-tree 90.9 ms ratio 0.65
The existing DPSS tests pass unchanged, including the SciPy golden values
and the production-scale N=60000 case at its original 1e-13 residual and
5e-12 orthogonality tolerances.
The clustering hazard is about the relative gap, not the absolute one.
The Slepian tridiagonal matrix has absolute gaps of order 1 that do not
shrink with N, so bisection isolates each eigenvalue comfortably; its
relative gaps fall to ~1e-9 at N=60000, so independent inverse iterations
would return vectors far less orthogonal than the tests require. All
wanted vectors are therefore reorthogonalized unconditionally as one
cluster, inside the iteration and once more at the end.
Three details that silently produce wrong answers:
- The Sturm pivot clamp must precede the sign test and the test must be
inclusive, or the count is wrong for an exactly zero pivot.
- The shifted solve cannot use a general banded factorization: T - theta*I
is deliberately near-singular, so a vanishing pivot is clamped rather
than treated as an error.
- The start vector is deterministic and reentrant, and avoids
std::uniform_real_distribution, whose output is implementation-defined
and would make tapers differ across standard libraries.
Adds four regression tests for the cases the clamps cover: a reducible
matrix, a diagonal matrix, the zero matrix, and Wilkinson W+21. The first
two do not use expectValidEigenpairs, which requires strictly descending
eigenvalues and a nonzero matrix norm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The editor owned 29 display widgets, gave each absolute bounds for a signal-chain layout, added them as children, then removed all of them again so the canvas could reparent and lay them out a second time. Every one of those setBounds calls was dead. Reaching the widgets needed `friend class SpectrumCanvas`, the destructor needed canvas.reset() to work around the resulting destruction order, and createNewCanvas had to re-push each value into a freshly built plot. The controls now belong to the canvas, which creates, lays out and reads them. What crosses the boundary is a value struct, not a widget. SpectrumDisplaySettings holds the state and the editor owns it, because VisualizerEditor creates the canvas lazily and may never create it at all. Saving a session that was never opened as a visualizer previously lost the drawer state, which was the only setting read back from the canvas; now nothing is read from widgets and the asymmetry is gone. It also carries the defaults and the 20 dB minimum span, which were duplicated across four sites, and sanitizes itself on load rather than trusting session XML. Deleted along the way: the friend declaration, the dead editor layout, the reparenting loops, the createNewCanvas re-push, the canvas's null-editor branches, and SpectrumViewerEditor's three listener bases. The editor goes from 742 lines to 131. Layout is now a wrapping flow rather than absolute x-positions. The rightmost drawer control used to end at x=1210, so any canvas narrower than about 1220 px silently clipped the capture status, and below about 1070 px the Options button overlapped the Y-units combo. Controls wrap to the next row instead, and the plot keeps a minimum size and scrolls rather than shrinking into unreadability. Visualizer already inherits Timer for its refresh callbacks, which only run during acquisition; capture and reference status must update when it is not, so that moves to a separate slower timer.
BUILD_PROCESSOR_TESTS has never worked. Three separate defects, none of them caught because the workflows do not run on feature branches and never set this option anyway. - Configure failed outright: Tests/Processor/CMakeLists.txt used the keyword form of target_link_libraries on the plugin while link_open_ephys_lib() uses the plain form, and CMake refuses to mix them on one target. - Link failed: the tests reach SpectrumCanvas, CanvasPlot, FrequencyPlot and PreparedSpectrumAnalysis across the plugin's DLL boundary, but only SpectrumViewer was marked TESTABLE. Marking the rest needs TestableExport.h, which is only reachable through the GUI's own umbrella headers, so the GUI Source directory joins the include path the way both test targets already have it. - The binary could not start: the GUI's test runtimes live in a per-configuration subdirectory under TestBin/common on a multi-config generator, so copying that directory nested a Release folder inside the output directory instead of placing the DLLs beside the executable. They are now located the same way their import libraries already were. The tests build and link after this. They still do not run on Windows: the process exits with STATUS_ENTRYPOINT_NOT_FOUND, a symbol mismatch between the plugin and the GUI's gui_testable_source library, which needs a fix on the GUI side rather than here.
Deselecting every channel during acquisition made updateRequestedInputRoute() return false, so rejectInputRouteReplacement() bumped the generation and left the old route and old runtime live. The callback kept enqueuing, the worker kept publishing frames for the previous channels, and beginSpectrumFrame kept refreshing the legend from those frames. The canvas went on drawing, and naming, channels the user had just deselected. The same held for an unavailable stream and for a stream with no Channels parameter. Tear the route down instead. The teardown has to run on the worker, which is the input route seqlock's only writer while acquisition runs and which owns activeAnalysis, so rejectInputRouteReplacement() now leaves a one-shot command and discardInvalidatedInputRoute() acts on it: publish an empty route to withdraw the callback's permission to enqueue, then release the runtime. Queued blocks keep the old generation and are rejected at the pipeline boundary, which is the path that already existed for stale blocks. A frozen capture is republished from the runtime's accumulator, so it cannot outlive the runtime and is discarded with it. The reference is a standalone snapshot and survives; its compatibility is re-evaluated against whatever is selected next. Report it as its own readiness state rather than as configurationFailed: nothing is broken, and the user can fix it by selecting something. A failed *build* still keeps the last good runtime live, which FailedStreamReplacementKeepsPreviousRouteLive covers - that distinction is deliberate. Also stop counting blocks that arrive against an empty route as rejected for an invalid channel mapping. They are not: the worker publishes an empty route before the first configuration and again after a teardown. Counting them as mapping errors made that diagnostic climb for as long as nothing was selected.
setCurrentCaptureAsReference() returns true once the request is published, but applying it is the worker's job and it can fail: if finalizeCapturedSpectrum() could not allocate the snapshot, applyReferenceRequest()'s exchange(0) consumed the request and neither branch ran. The request vanished. Meanwhile the canvas had already switched to Fine and Overlay on the click, so the user was left looking at a comparison mode with "No reference" beside it and no indication that anything had gone wrong. Count the drops, expose the count, and have the canvas latch it: the status reads "Reference unavailable" with a tooltip saying to capture again, and the comparison mode goes back to absolute rather than staying on a mode that does nothing. The latch clears on the next set or clear. The drop itself needs the allocation in finalizeCapturedSpectrum() to fail, which no public entry point can provoke, so the test covers what makes the counter trustworthy as a UI signal instead: the paths a user actually takes never move it. Also correct the header's "session-local reference" wording. The reference is released with the rest of the acquisition state on every stop, so it is acquisition-run-local, which is what the canvas tooltip already says.
galenlynch
marked this pull request as ready for review
September 17, 2026 18:07
Collaborator
Author
|
Thanks for taking the time to audit and improve this! The routing, shutdown, memory-ordering, invalid-selection, reference-status, and canvas-state changes all look useful. I would prefer to revert the in-tree eigensolver and OpenBLAS-removal commits, however. Spectrum Viewer uses only a small part of OpenBLAS, but its practical packaging cost is modest: the Windows runtime is about 28 MB installed and 4 MB compressed, while Linux and macOS link it statically. DPSS preparation is also asynchronous and infrequent, so the reported reduction in preparation time has little user-facing effect. Retaining OpenBLAS lets us continue using LAPACK's established selected-tridiagonal solver instead of assuming responsibility for approximately 570 lines of numerical code. I also noticed a possible convergence issue. The existing production DPSS tests pass, so this does not show that the generated tapers are currently wrong. It does demonstrate the additional validation burden of maintaining the solver ourselves. Given the modest operational benefit of removing OpenBLAS, I think the established LAPACK path is the safer and simpler choice. I also found two smaller integration issues:
Once those are resolved, I think the new canvas ownership and layout changes should get one additional graphical pass because they were added after the previous GUI testing. I'd be happy to do that graphical testing if you haven't already. |
Anchor a capture's sample span on the windows it included rather than on arriving input blocks. Source sample numbering is not guaranteed to run forward across a discontinuity - a looping File Reader restarts it - which left the block-derived anchor ahead of every window that followed. Both CapturedSpectrum's constructor and SpectrumFrameFifo::tryPushReduced reject a span that does not run forward, so a completed capture was thrown away with "Captured spectrum metadata is invalid", the progress preview was silently dropped, and the elapsed readout went negative. Also stop shedding capture windows. Backlog shedding is only sound for live display frames; a shed capture window is data the average never sees, so discarding one stalls the capture instead of helping it catch up. Raise INPUT_QUEUE_CAPACITY to 32 blocks so an estimate-reduce-baseline burst cannot overflow the queue, since a dropped block resets window history and costs a whole 2 s Fine window.
The stall test compared the current residual against bestResidual, which an improving iteration had just assigned that same value, so the guard reduced to x > 0.9x and was true for any positive residual. Refinement always exited at iteration 1. That path also set converged = true, so the iteration cap was never reached and the unconverged count could only be incremented by the degenerate-length break - solverInfo was structurally pinned at zero and could not report anything. Output was unaffected: two solves against a shift bisected to 2*eps*||T|| is enough here, and the SciPy golden values are unchanged at every N including 60000. What was broken is the reporting path, which is what would catch a future regression. Compare against the previous iterate instead, and stop conflating a stall with convergence. Stalling is a legitimate exit - the shift error floors the residual - so acceptance is now decided once, after the loop, on the residual actually achieved. That needs two thresholds rather than one: a refinement floor of 4*eps where further solves only add rounding, and a separate acceptance bar of 1e-13 that a vector has to clear. The previous single threshold was documented as deliberately near-unreachable, which is correct for a stop condition but cannot serve as an acceptance bar. Raise the cap from 8 to 16 solves; the measurements show it is never approached. Measured solves and largest residual: general tridiagonal N=128 K=6, 9 solves at 7.1e-16; DPSS Slepian N=60000 K=5, 12 solves at 2.5e-15; Wilkinson W+21 K=6, 11 solves at 6.3e-16. 115/115 tests pass.
anjaldoshi
force-pushed
the
real-time-spectrum-refactor
branch
from
September 18, 2026 23:40
85f4f14 to
a57fda7
Compare
galenlynch
force-pushed
the
real-time-spectrum-refactor
branch
from
September 20, 2026 04:34
fb26e6a to
a57fda7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
not ready to be merged: need to test with the GUI
Summary
FIFO and worker-owned circular history. Callbacks now copy each selected
channel once and publish or drop the complete multichannel block.
multitaper PSD estimation using runtime-generated DPSS tapers and persistent
float FFTW plans.
PSD/ASD units, linear or logarithmic frequency mapping, cursor values, and a
worker-side mean-plus-peak pixel reduction.
stable fixed/automatic dB ranges, low-variance Fine capture, and session
reference overlay/delta comparison.
message thread. Remove the unused legacy spectrum pipeline.
Why
The released implementation can discard callback tails, join samples across
gaps, and publish channels independently. It also plots an unnormalized squared
FFT magnitude after several undocumented smoothing stages. The result depends
on callback partitioning, window length, sample rate, and display range, which
makes comparisons unreliable during rig debugging.
This change gives every accepted block and spectrum an explicit sample range,
configuration generation, channel map, estimator description, and physical
unit. Expensive allocation, DPSS generation, and FFT planning occur on a
dedicated configuration thread while the previous valid runtime remains live.
Validation
detrending, leakage, noise density, channel isolation, and odd/even lengths.
overflow, backlog shedding, asynchronous replacement, failed preparation,
stop/restart races, capture, references, and display metadata.
0.45 ms Fast, 1.04 ms Balanced, and 4.67 ms Fine at the median. Software
repaint medians were 2.75–5.54 ms. These are local measurements, not target-rig
acceptance results.
Dependencies and remaining validation
OpenEphysFFTW and corresponding cross-platform
fftw3fbinaries.OpenBLAS backend; hosted Windows and universal-macOS builds remain required.
latency measurements remain before requesting final review.
scientific work; this PR keeps the raw calibrated spectrum visible.
Attribution
Defensive stream/range checks and the MSVC exception setting were adapted from
Anjal Doshi's
refactor-optimizationbranch. The relevant commits preserveco-authorship.