Skip to content

Restore granular feedback behind a show_detail parameter - #26

Open
peterbjohnson wants to merge 1 commit into
22-organise-feedback-messagesfrom
24-show-detail-feedback
Open

Restore granular feedback behind a show_detail parameter#26
peterbjohnson wants to merge 1 commit into
22-organise-feedback-messagesfrom
24-show-detail-feedback

Conversation

@peterbjohnson

Copy link
Copy Markdown
Member

Closes #24.

Based on 22-organise-feedback-messages (#23) so the diff shows only this change; GitHub will retarget it to main when #23 merges.

Summary vs detail, not current vs old. polished_feedback_messagesummary_feedback, generate_feedback_messagedetail_feedback, and the two blocks in feedback_messages.py are relabelled to match. build_feedback composes them: the summary is always shown, the detail is appended only when show_detail is on.

SHOW_DETAIL = False sits with the other module-level defaults and is teacher-configurable via params["show_detail"], like the thresholds.

The caveat. detail_caveat_message introduces the detail section. It is worded to hold for MIDI submissions too — those aren't transcribed at all, so a flat "this came from a transcription" would be inaccurate about half the time:

The per-note comments below are generated automatically and may not be accurate for every note, especially if your recording was transcribed from audio.

Duration direction bug. event_level_feedback stored duration_abs_diff = abs(...), so direction = "longer" if duration_abs_diff > 0 was always true — a note held too short was reported as too long. Rather than make the existing key signed, which would have silently inverted duration_correct (the threshold compares duration_relative_diff <= threshold, and a signed value would let badly short notes pass), the fix adds a separate duration_signed_diff and leaves the threshold logic untouched. Two tests cover both directions.

Part 1 dropped from detail_feedback. It restated the summary in the older voice. The overview_* constants went with it.

Tests. Six new, in a section 11: detail off by default, appended below the summary when on, introduced by the caveat, reachable through params, and the two duration-direction cases. 78 pass. Section titles and the caveat are asserted by constant, following 6121639.

Notebook. Phase1-1.5_summary.ipynb cells 3/40/42 are updated to the new names. Outputs are not re-run, so the printed detail output in that notebook still shows the old Part 1 overview.

🤖 Generated with Claude Code

The per-note renderer was superseded rather than removed, leaving two
renderers with no way to reach the detailed one. Reframe the pair as
summary vs detail and let the caller ask for both.

- generate_feedback_message -> detail_feedback,
  polished_feedback_message -> summary_feedback, and the same split in
  feedback_messages.py.
- build_feedback composes them: the summary is always shown, the detail
  is appended when show_detail is on. SHOW_DETAIL defaults to False and
  is teacher-configurable through params.
- detail_caveat_message introduces the detail section, so students know
  the per-note claims may not be accurate.
- detail_feedback loses its Part 1 overview, which restated the summary.
  The overview_* messages go with it.
- Fix the duration direction: duration_abs_diff was made absolute at the
  point of measurement, so the detail message always read "longer" and
  never "shorter". event_level_feedback now also records
  duration_signed_diff, and the threshold logic is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@ada-jz7125 ada-jz7125 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the notebook may need to be rerun later, I realised that text assertions are fixed in PR27

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this notebook may need to be rerun later to make sure the output is up-to-date

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.

2 participants