Skip to content

makelab: fix plotting bugs, prune dead code, add docs + audio tests#11

Merged
jonfroehlich merged 1 commit into
masterfrom
signals-v2-makelab-review
Jun 24, 2026
Merged

makelab: fix plotting bugs, prune dead code, add docs + audio tests#11
jonfroehlich merged 1 commit into
masterfrom
signals-v2-makelab-review

Conversation

@jonfroehlich

Copy link
Copy Markdown
Member

What

Review and cleanup of the makelab helper library behind the signals tutorials. Scoped deliberately to the .py files — no notebooks were changed.

Correctness fixes

  • plot_audio now passes its computed bit-depth title through to plot_signal (it was building the title then discarding it, so the "{bits}-bit, {rate} Hz audio" title never appeared).
  • plot_spectrogram default title: duration is len(s) / sampling_rate, not len(s) * sampling_rate.
  • == Noneis None in plot_signal / plot_spectrogram.
  • set_xticks() before set_xticklabels() everywhere, eliminating the matplotlib FixedFormatter warning students would otherwise see.
  • convert_to_mono uses mean(axis=1) — correct for any channel count, not just stereo (old sum(axis=1)/2 assumed exactly two channels).

Code-quality cleanup

  • Dropped unused scipy / librosa / distance imports; the scipy.signal import was being shadowed by local signal variables.
  • De-duplicated map/remap (map is now a thin alias of remap).
  • Removed a dead variable and simplified the tangled min_gap guard in calc_zero_crossings (output unchanged — pinned by existing tests).
  • Hoisted the mid-file matplotlib.ticker import to the top.

Pedagogy

  • Standardized docstrings with Parameters: / Returns:; fixed stale comments/typos (plot_Signal, y=1.14, a non-existent "quantization level" param).
  • Added pointers to library equivalents (numpy.interp, scipy.ndimage.shift, librosa.zero_crossings, librosa.to_mono) with the behavioral differences noted — keeping the hand-written versions as the teaching artifact.
  • Added Tutorials/makelab/README.md documenting the package purpose, import/CWD rules, and the public API.

Tests

  • Added tests/test_makelab_audio.py (none existed), including a >2-channel case that guards the convert_to_mono fix.

Verification

  • pytest tests/29 passed
  • Agg render smoke of all plot helpers under warnings-as-errors → clean
  • pytest --nbmake on the three signals notebooks → 3 passed (17m19s)

Follow-up

🤖 Generated with Claude Code

Review/cleanup of the makelab helper library (scoped to the .py files;
notebooks untouched).

Correctness:
- plot_audio now passes its computed bit-depth title to plot_signal
  (previously discarded, so the title never appeared)
- plot_spectrogram default title: duration is len(s)/sampling_rate,
  not len(s)*sampling_rate
- == None -> is None in plot_signal/plot_spectrogram
- set_xticks() before set_xticklabels() everywhere to avoid the
  matplotlib FixedFormatter warning students would otherwise see
- convert_to_mono uses mean(axis=1) (correct for any channel count,
  not just stereo)

Cleanup:
- drop unused scipy/librosa/distance imports; the scipy.signal import
  was shadowed by local `signal` vars
- de-duplicate map/remap (map is now a thin alias of remap)
- remove dead last_zero_crossing_idx var; simplify the min_gap guard
- hoist matplotlib.ticker import to the top

Pedagogy:
- standardize docstrings with Parameters/Returns; fix stale comments
- add pointers to library equivalents (numpy.interp, scipy.ndimage.shift,
  librosa.zero_crossings, librosa.to_mono) noting behavioral differences
- add Tutorials/makelab/README.md documenting the package + public API

Tests:
- add tests/test_makelab_audio.py (incl. a >2-channel case that guards
  the convert_to_mono fix)

Verified: pytest tests/ (29 passed); Agg render smoke of all plot
helpers with warnings-as-errors (clean); nbmake on the three signals
notebooks (3 passed).

Notebook-side follow-up tracked in #10.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jonfroehlich jonfroehlich merged commit 493c9dc into master Jun 24, 2026
2 checks passed
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.

1 participant