refactor: both rebound compression scatter and histogram use the same…#169
Conversation
… data. refactor: added a lower bound to stop noise populating the suspension data
There was a problem hiding this comment.
Code Review
This pull request refactors suspension telemetry analysis by introducing a centralized filterLowActivityOutliers function in run-analysis.ts to filter out statistical outliers and low-activity events. This function is integrated into both ReboundCompressionPlot and VelocityHistogram to ensure consistent data filtering across graphs. Additionally, unused velocity sampling utilities were removed from telemetryUtils.ts, and the velocity histogram now supports auto-scaling. The review feedback highlights a potential bug where a standard deviation of zero would incorrectly filter out all data points, and suggests implementing a caching mechanism using a WeakMap to optimize the performance of the heavy filtering computations in VelocityHistogram.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return items.filter((a) => { | ||
| const passesStdDev = | ||
| Math.abs(a.velocity - vMean) < 5 * vStd && | ||
| Math.abs(a.displacement - dMean) < 5 * dStd | ||
|
|
||
| // Drop only low-activity points: slow AND small relative to full travel. | ||
| const isLowActivity = | ||
| Math.abs(a.velocity) < minVelocity && | ||
| Math.abs(a.displacement) < minDisplacement | ||
|
|
||
| return passesStdDev && !isLowActivity | ||
| }) |
There was a problem hiding this comment.
If all elements in items have the same velocity or displacement (for example, if there is only one item in the array), the standard deviation (vStd or dStd) will be 0. In this case, the expression Math.abs(a.velocity - vMean) < 5 * vStd evaluates to 0 < 0 (which is false), causing all items to be filtered out. We should handle the case where the standard deviation is 0 to prevent valid data from being incorrectly discarded.
| return items.filter((a) => { | |
| const passesStdDev = | |
| Math.abs(a.velocity - vMean) < 5 * vStd && | |
| Math.abs(a.displacement - dMean) < 5 * dStd | |
| // Drop only low-activity points: slow AND small relative to full travel. | |
| const isLowActivity = | |
| Math.abs(a.velocity) < minVelocity && | |
| Math.abs(a.displacement) < minDisplacement | |
| return passesStdDev && !isLowActivity | |
| }) | |
| return items.filter((a) => { | |
| const passesStdDev = | |
| (vStd === 0 || Math.abs(a.velocity - vMean) < 5 * vStd) && | |
| (dStd === 0 || Math.abs(a.displacement - dMean) < 5 * dStd) | |
| // Drop only low-activity points: slow AND small relative to full travel. | |
| const isLowActivity = | |
| Math.abs(a.velocity) < minVelocity && | |
| Math.abs(a.displacement) < minDisplacement | |
| return passesStdDev && !isLowActivity | |
| }) |
| function buildFilteredVelocities(seriesItem: VelocityHistogramSeries): number[] { | ||
| const length = seriesItem.length ?? 220; | ||
| const min = seriesItem.min ?? 0; | ||
| const max = seriesItem.max ?? 4096; | ||
|
|
||
| const activities = processCompressions( | ||
| seriesItem.rawData, | ||
| seriesItem.freq, | ||
| length, | ||
| min, | ||
| max, | ||
| ); | ||
|
|
||
| // Filter compression and rebound subsets separately, exactly as the scatter | ||
| // does, then combine so the histogram bins precisely the scatter's points. | ||
| const kept = [ | ||
| ...filterLowActivityOutliers( | ||
| activities.filter((a) => a.type === "compression"), | ||
| length, | ||
| ), | ||
| ...filterLowActivityOutliers( | ||
| activities.filter((a) => a.type === "rebound"), | ||
| length, | ||
| ), | ||
| ]; | ||
|
|
||
| return kept.map((a) => a.velocity); | ||
| } |
There was a problem hiding this comment.
The buildFilteredVelocities function performs heavy computations (including processCompressions and multiple array filtering/mapping operations) on telemetry data. Since the series prop is often recreated on every render by parent components, useMemo will re-run this function frequently, leading to UI lag. We can introduce a WeakMap cache keyed by the stable rawData array reference to cache the computed velocities and avoid redundant calculations.
const velocityCache = new WeakMap<
RawSuspensionData[],
{
freq: number;
length: number;
min: number;
max: number;
result: number[];
}
>();
function buildFilteredVelocities(seriesItem: VelocityHistogramSeries): number[] {
const length = seriesItem.length ?? 220;
const min = seriesItem.min ?? 0;
const max = seriesItem.max ?? 4096;
const freq = seriesItem.freq;
const cached = velocityCache.get(seriesItem.rawData);
if (
cached &&
cached.freq === freq &&
cached.length === length &&
cached.min === min &&
cached.max === max
) {
return cached.result;
}
const activities = processCompressions(
seriesItem.rawData,
freq,
length,
min,
max,
);
// Filter compression and rebound subsets separately, exactly as the scatter
// does, then combine so the histogram bins precisely the scatter's points.
const kept = [
...filterLowActivityOutliers(
activities.filter((a) => a.type === "compression"),
length,
),
...filterLowActivityOutliers(
activities.filter((a) => a.type === "rebound"),
length,
),
];
const result = kept.map((a) => a.velocity);
velocityCache.set(seriesItem.rawData, { freq, length, min, max, result });
return result;
}
… data.
refactor: added a lower bound to stop noise populating the suspension data