Fix shaper.vals/calibration_data.freqs length mismatch on some Kalico builds - #282
Open
Hannott wants to merge 2 commits into
Open
Fix shaper.vals/calibration_data.freqs length mismatch on some Kalico builds#282Hannott wants to merge 2 commits into
Hannott wants to merge 2 commits into
Conversation
… builds ShaperComputation assumed that on Klipper/Kalico versions without a freq_bins field on CalibrationResult, shaper.vals is already truncated to exactly match calibration_data.freqs. That only holds when the requested max_freq is >= the shaper search's own frequency ceiling (MAX_SHAPER_FREQ, 150Hz on KalicoCrew/kalico) -- fit_shaper inflates its internal max_freq to max(max_freq, test_freqs.max()), so a lower requested max_freq gets silently overridden and shaper.vals ends up longer than calibration_data.freqs. This reached matplotlib as an opaque "x and y must have same first dimension" crash during graph generation. Since freq_bins is sorted ascending, both truncations are prefixes of the same array, so slicing shaper.vals down to the length we need recovers the correct values without needing to know the installed Kalico/Klipper version's internal behavior. Falls back to edge-padding with a warning in case vals is ever shorter than expected instead of crashing. Also fixes traceback.print_exc() being used inside an f-string in the graph generation error handler -- it prints to stderr and returns None, which is why that error message always had a trailing "None" line instead of the actual traceback. Adds test_shaper_length_mismatch.py, which reproduces the crash against a real Kalico checkout with a synthetic capture (fails on max_freq below 150 on the old code, passes across max_freq 80-300 with this fix).
Contributor
Reviewer's GuideFixes a crash in SHAPER_CALIBRATE graph generation caused by a length mismatch between shaper.vals and calibration_data.freqs on older Klipper/Kalico builds with low max_freq, and improves error reporting plus adds a regression test that drives real Kalico firmware across multiple max_freq values. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="shaketune/graph_creators/computations/shaper_computation.py" line_range="120-127" />
<code_context>
+ # (a mismatch here used to reach matplotlib as an opaque "x and y must have the same
+ # first dimension" crash -- see issue with max_freq=300 on KalicoCrew/kalico).
+ n = len(calibration_data.freqs)
+ if len(shaper.vals) < n:
+ ConsoleOutput.print(
+ f'Warning: {shaper.name} returned fewer frequency bins than expected '
+ f'({len(shaper.vals)} < {n}); padding with its last value. This may indicate '
+ 'an unsupported Klipper/Kalico version -- results near the high end of the '
+ 'graph may be inaccurate.'
+ )
+ vals_resampled = np.pad(shaper.vals, (0, n - len(shaper.vals)), mode='edge')
+ else:
+ vals_resampled = shaper.vals[:n]
</code_context>
<issue_to_address>
**issue:** Guard against the case where `shaper.vals` is empty before using `np.pad(..., mode='edge')`.
If `shaper.vals` is ever empty, `np.pad(..., mode='edge')` will raise a `ValueError` because there is no edge element to replicate. Please add an explicit check for the empty case (e.g. `if not shaper.vals: ...`) or use `mode='constant'` for this branch so a Klipper/Kalico mismatch doesn’t become a hard crash.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
np.pad with mode='edge' has no element to replicate when the input array is empty, so it raises ValueError instead of the graceful fallback this was meant to be. Handle len(shaper.vals) == 0 explicitly with a zero-filled array instead. Addresses Sourcery review comment on Frix-x#282.
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
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.
Summary
SHAPER_CALIBRATEgraph generation can crash with aValueError: x and y must have same first dimension, but have shapes (N,) and (M,)matplotlib error, reported onKalicoCrew/kalico:mainwhen a lowermax_freqis configured (reproduced here withmax_freq < 150).Root cause:
ShaperComputation.compute()assumes that on Klipper/Kalico builds whereCalibrationResulthas nofreq_binsfield,shaper.valsis already truncated to exactly matchcalibration_data.freqs. That assumption only holds when the requestedmax_freqis >= the shaper search's own internal frequency ceiling (MAX_SHAPER_FREQ, 150Hz onKalicoCrew/kalico). Internally,fit_shapercomputesmax_freq = max(max_freq, test_freqs.max())-- so a lower requestedmax_freqgets silently overridden, andshaper.valsends up longer thancalibration_data.freqs, which was truncated with the original (lower) value. That length mismatch reaches matplotlib as an opaque crash during graph generation.Fix
Since
freq_binsis sorted ascending, both truncations are prefixes of the same underlying array -- slicingshaper.valsdown tolen(calibration_data.freqs)recovers the correct values without needing to know the installed Kalico/Klipper version's internal behavior. Falls back to edge-padding with a console warning in the (currently unobserved) case wherevalsis shorter than expected, rather than crashing.Also fixes
traceback.print_exc()being used inside an f-string in the graph-generation error handler (shaketune_process.py) -- it prints to stderr and returnsNone, which is why that error message always had a trailing literalNoneinstead of the actual traceback.Testing
Added
test_shaper_length_mismatch.py, which drives the realShaperComputationagainst a real Kalico checkout (KalicoCrew/kalico:main) with a synthetic capture, acrossmax_freqvalues from 80-300:max_freq80 and 100 (belowMAX_SHAPER_FREQ) with the exact length-mismatch pattern; also reproduced the literal matplotlib crash end-to-end viashaketune.cli.max_freqvalues, no regressions at the values that already worked.Also verified with
ruff check/ruff format --check(both files clean).Summary by Sourcery
Handle length mismatches between shaper.vals and calibration_data.freqs on older Klipper/Kalico builds to prevent graph-generation crashes.
Bug Fixes:
Tests: