Bugfix for PR #820 Added power spectral density curves at the spectrum chart #825
Conversation
Co-authored-by: MikeNomatter <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update corrects typographical errors in the Changes
Sequence Diagram(s)(No sequence diagram generated as the changes are typographical and do not affect control flow.) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/graph_spectrum.js (1)
190-199: PSD is recomputed on every Y-zoom “input” event – consider throttling or caching
dataLoad()performs a full Welch PSD every time the vertical-zoom slider fires (even when the slider is just being dragged).
Although the handler is debounced at 100 ms, a large log file + 50 % overlap FFT can still block the UI for noticeable time on low-end CPUs.If you observe jank, consider:
- if (userSettings.spectrumType == SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY) { - dataLoad(); - GraphSpectrumPlot.setData(fftData, userSettings.spectrumType); - } + if (userSettings.spectrumType === SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY) { + window.requestIdleCallback(() => { + dataLoad(); + GraphSpectrumPlot.setData(fftData, userSettings.spectrumType); + that.refresh(); + }, {timeout: 100}); + return; // skip extra refresh below – the callback will draw + }(works in all Chromium-based browsers; Firefox has a ponyfill.)
src/graph_spectrum_plot.js (1)
1075-1088: Renamed parameter breaks readability; consider keeping localTICKSYou replaced the internal constant
TICKSwith a parameterticks.
This works, but many call-sites still pass nothing, relying on the default.
For clarity and to avoid accidental0values, keep the local constant:-GraphSpectrumPlot._drawVerticalGridLines = function (…, label, ticks = 5) { +GraphSpectrumPlot._drawVerticalGridLines = function (…, label, ticks = 5) { + const TICKS = ticks; // semantic aliasVery minor, but eases grepping.
Also applies to: 1092-1100, 1106-1107
src/graph_spectrum_calc.js (2)
112-121: Zoom→segment-size logic is reversed & typo inmultipiler
multiplier = floor(1 / zoomY)makes the segment length smaller when the
user zooms out (zoomY > 1). Welch PSD benefits from more points per
segment for better resolution when zooming out.Consider:
-const multiplier = Math.floor(analyserZoomY); +const multiplier = Math.floor(analyserZoomY);(and spell “multiplier”).
623-628: Shadowing outer loop index with inneriThe inner
for (let i = 0; …)hides the outeri, which can be confusing
during debugging.Rename one of them, e.g.:
-for (let i = 0; i < n_per_seg / 2; i++) { +for (let k = 0; k < n_per_seg / 2; k++) { const re = fftComplex[2 * k]; const im = fftComplex[2 * k + 1]; magnitudes[k] = Math.hypot(re, im); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(1 hunks)src/graph_spectrum.js(2 hunks)src/graph_spectrum_calc.js(4 hunks)src/graph_spectrum_plot.js(10 hunks)
🔇 Additional comments (1)
index.html (1)
462-462: Confirm option value matches JavaScript mapping
Verify that the new<option value="4">Power spectral density</option>aligns with thePOWER_SPECTRAL_DENSITY = 4constant in your spectrum plotting code (e.g.,graph_spectrum_plot.js) and that the event handler forspectrumTypeSelecthas been updated to handle this new value.
| const WIDTH = canvasCtx.canvas.width - ACTUAL_MARGIN_LEFT; | ||
| const LEFT = canvasCtx.canvas.offsetLeft + ACTUAL_MARGIN_LEFT; | ||
| const TOP = canvasCtx.canvas.offsetTop; | ||
|
|
There was a problem hiding this comment.
offsetLeft/Top are 0 on off-screen canvases – translation becomes NaN
_drawPowerSpectralDensityGraph is executed first on the cached, detached
<canvas> created in _drawCachedElements.
cachedCtx.canvas.offsetLeft / offsetTop are undefined, so
const LEFT = canvasCtx.canvas.offsetLeft + ACTUAL_MARGIN_LEFT; // NaN
const TOP = canvasCtx.canvas.offsetTop;
canvasCtx.translate(LEFT, TOP); // ⟶ translate(NaN, undefined)silently aborts all subsequent drawing in Chrome.
Fix: drop the DOM offsets (they are already compensated by the parent
translate in non-cached path):
-const LEFT = canvasCtx.canvas.offsetLeft + ACTUAL_MARGIN_LEFT;
-const TOP = canvasCtx.canvas.offsetTop;
+const LEFT = ACTUAL_MARGIN_LEFT;
+const TOP = 0;| canvasCtx.moveTo(0, 0); | ||
| for (let pointNum = 0; pointNum < pointsCount; pointNum++) { | ||
| const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount; | ||
| const y = HEIGHT - (this._fftData.psdOutput[pointNum] - minY) * scaleY; | ||
| canvasCtx.lineTo(freq * scaleX, y); | ||
| } | ||
| canvasCtx.stroke(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The initial moveTo(0, 0) draws a stray line
moveTo(0,0) precedes the loop that starts at the first PSD point, so a
diagonal is rendered from the origin to the first data point.
Just start the path at the first point:
-canvasCtx.moveTo(0, 0);
-for (let pointNum = 0; pointNum < pointsCount; pointNum++) {
- const freq = /* … */;
- const y = /* … */;
- canvasCtx.lineTo(freq * scaleX, y);
-}
+const y0 = HEIGHT - (this._fftData.psdOutput[0] - minY) * scaleY;
+canvasCtx.moveTo(0, y0);
+for (let pointNum = 1; pointNum < pointsCount; pointNum++) {
+ const freq = /* … */;
+ const y = HEIGHT - (this._fftData.psdOutput[pointNum] - minY) * scaleY;
+ canvasCtx.lineTo(freq * scaleX, y);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canvasCtx.moveTo(0, 0); | |
| for (let pointNum = 0; pointNum < pointsCount; pointNum++) { | |
| const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount; | |
| const y = HEIGHT - (this._fftData.psdOutput[pointNum] - minY) * scaleY; | |
| canvasCtx.lineTo(freq * scaleX, y); | |
| } | |
| canvasCtx.stroke(); | |
| // Start drawing at the first data point instead of (0,0) | |
| const y0 = HEIGHT - (this._fftData.psdOutput[0] - minY) * scaleY; | |
| canvasCtx.moveTo(0, y0); | |
| for (let pointNum = 1; pointNum < pointsCount; pointNum++) { | |
| const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount; | |
| const y = HEIGHT - (this._fftData.psdOutput[pointNum] - minY) * scaleY; | |
| canvasCtx.lineTo(freq * scaleX, y); | |
| } | |
| canvasCtx.stroke(); |
| GraphSpectrumCalc._fft_segmented = function(samples, n_per_seg, n_overlap) { | ||
| const samplesCount = samples.length; | ||
| let output = []; | ||
| for (let i = 0; i < samplesCount - n_per_seg; i += n_per_seg - n_overlap) { | ||
| const fftInput = samples.slice(i, i + n_per_seg); | ||
|
|
||
| if (userSettings.analyserHanning) { | ||
| this._hanningWindow(fftInput, n_per_seg); | ||
| } | ||
|
|
||
| const fftComplex = this._fft(fftInput); | ||
| const magnitudes = new Float64Array(n_per_seg / 2); | ||
| for (let i = 0; i < n_per_seg / 2; i++) { | ||
| const re = fftComplex[2 * i]; | ||
| const im = fftComplex[2 * i + 1]; | ||
| magnitudes[i] = Math.hypot(re, im); | ||
| } | ||
| output.push(magnitudes); | ||
| } | ||
|
|
||
| return output; |
There was a problem hiding this comment.
No PSD segments generated when pointsPerSegment == samples.length
Termination condition uses strict <:
for (let i = 0; i < samplesCount - n_per_seg; i += n_per_seg - n_overlap)If the log is shorter than the default 512 pts (or pointsPerSegment
was clamped to samples.length), the loop never executes, leading to
segmentsCount === 0, an empty PSD, and downstream NaNs in the plot.
Fix:
-for (let start = 0; start < samplesCount - n_per_seg; start += n_per_seg - n_overlap) {
+for (let start = 0; start <= samplesCount - n_per_seg; start += n_per_seg - n_overlap) {Optionally guard against n_per_seg > samplesCount by reducing
n_per_seg to samplesCount.
demvlad
force-pushed
the
psd_spectrum_bugfix
branch
from
b3c4abf to
69a4288
Compare
|
Checked! |
demvlad
force-pushed
the
psd_spectrum_bugfix
branch
from
69a4288 to
32b0086
Compare
|
Bugfix for PR #820 Added power spectral density curves at the spectrum chart
Summary by CodeRabbit
Style
Chores