mir_eval icon indicating copy to clipboard operation
mir_eval copied to clipboard

Chord display

Open bmcfee opened this issue 6 months ago • 8 comments

Implements #424

This PR adds mir_eval.display.chords() and related functionality for visualizing chord annotations.

It is largely based on the segments function, and there is plenty of redundancy in the code here. However, there are also enough differences that it's easier to duplicate than refactor.

The text annotation functionality is also fully in place, but I did not add test coverage for this yet as it currently depends on some slightly broken behavior in matplotlib that is likely to change at some point. (See discussion in #424 ) As such, I don't mind a temporary coverage reduction.

Finally, this PR also registers six colormaps in the matplotlib registry when the module is imported:

  • pitch
  • pitch_light
  • pitch_dark
  • fifths
  • fifths_light
  • fifths_dark

The default is fifths for chord display, and otherwise the pitch modes are not used. However, I think we can probably find some use for them later on, perhaps in some kind of rainbow piano roll viz or something. (I've experimented with this a little offline, but nothing too compelling has emerged yet.)

bmcfee avatar Aug 04 '25 19:08 bmcfee

One nitpick that I'm noticing just now: I'm not 100% satisfied with the hatching on sus4 chords. The final segment in our generated image comparison test: https://github.com/mir-evaluation/mir_eval/blob/4cac1405a7cf428e27fa9698c348348607672987/tests/baseline_images/test_display/test_display_chord_nolabels.png is a sus4, and it has the unique problem that it is visually indistinguishable from a sequence of alternating major/other chords.

I'm entertaining suggestions on how else we could render sus4.

bmcfee avatar Aug 04 '25 19:08 bmcfee

Codecov Report

:x: Patch coverage is 86.02151% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
mir_eval/display.py 86.02% 13 Missing :warning:
Flag Coverage Δ
unittests 85.64% <86.02%> (-0.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
mir_eval/display.py 78.93% <86.02%> (+1.87%) :arrow_up:

... and 4 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Aug 04 '25 19:08 codecov[bot]

Bumping minimal matplotlib to 3.6 so as to handle colormap registration.

bmcfee avatar Aug 04 '25 19:08 bmcfee

Had to nudge the minimal matplotlib here, which has set off a chain of environment updates and other painful nonsense.

Meanwhile, I've also thrown in a workflow step to do an upload artifact for failing image comparison tests. We recently added this over in librosa and it's a huge time saver.

bmcfee avatar Aug 04 '25 19:08 bmcfee

It appears that my use of hatch_linewidth requires matplotlib>=3.10. This is quite a bummer, as I think it looks much better with thick hatches. Here's the default (thin) version:

test_display_chord_nolabels

bmcfee avatar Aug 05 '25 16:08 bmcfee

Honestly the thick hatch version is not so bad. Not sure whether it's worth doing something conditional on matplotlib version.

I think there are other hatches available that could be used for sus4, like stars etc., but I guess they might look terrible.

craffel avatar Aug 06 '25 16:08 craffel

I think there are other hatches available that could be used for sus4, like stars etc., but I guess they might look terrible.

Another option could be to use o for suspended chords, and then use - or | to identify if it's sus2 or sus4. But again, image

bmcfee avatar Aug 06 '25 17:08 bmcfee

Clownpants ftw

craffel avatar Aug 06 '25 17:08 craffel