Skip to content

Chord display#425

Merged
craffel merged 24 commits intomainfrom
chord-display
Feb 19, 2026
Merged

Chord display#425
craffel merged 24 commits intomainfrom
chord-display

Conversation

@bmcfee
Copy link
Copy Markdown
Collaborator

@bmcfee bmcfee commented Aug 4, 2025

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
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Aug 4, 2025

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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 4, 2025

Codecov Report

❌ Patch coverage is 87.65432% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
mir_eval/display.py 87.34% 10 Missing ⚠️
Flag Coverage Δ
unittests 85.66% <87.65%> (+0.01%) ⬆️

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

Files with missing lines Coverage Δ
mir_eval/beat.py 99.06% <ø> (ø)
mir_eval/melody.py 93.16% <100.00%> (-0.05%) ⬇️
mir_eval/multipitch.py 86.36% <ø> (ø)
mir_eval/onset.py 100.00% <100.00%> (ø)
mir_eval/separation.py 9.49% <ø> (ø)
mir_eval/transcription.py 100.00% <ø> (ø)
mir_eval/display.py 78.94% <87.34%> (+1.88%) ⬆️

... and 4 files with indirect coverage changes

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

@bmcfee
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Aug 4, 2025

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

@bmcfee
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Aug 4, 2025

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
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Aug 5, 2025

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

@craffel
Copy link
Copy Markdown
Collaborator

craffel commented Aug 6, 2025

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.

@bmcfee
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Aug 6, 2025

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

@craffel
Copy link
Copy Markdown
Collaborator

craffel commented Aug 6, 2025

Clownpants ftw

@bmcfee bmcfee requested a review from Copilot February 19, 2026 16:21
@bmcfee
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Feb 19, 2026

I lost steam on this one at the end of the summer, and am now coming back to try to wrap it up.

Not sure whether it's worth doing something conditional on matplotlib version.

I'd rather not go that far with it, since that's getting into rather non-idiomatic mpl usage. (Even moreso than we already are.) We can give an example docstring that shows how to make it a little prettier on recent mpl versions though.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds chord-annotation visualization support to mir_eval.display, including custom pitch-/fifths-based colormaps and hatch patterns, and updates test/CI/dependency configuration to support and validate the new visualization behavior.

Changes:

  • Introduce mir_eval.display.chords() plus chord-oriented colormap registration and hatch styling.
  • Add a new pytest-mpl image test and baseline image for chord visualization.
  • Bump minimum supported versions (Python/numpy/matplotlib) and update CI + conda environments accordingly (including uploading mpl diff artifacts on failure).

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
mir_eval/display.py Adds chord display plotting, registers new categorical colormaps, and defines hatch patterns for chord qualities.
tests/test_display.py Adds an mpl image comparison test for chord visualization across pitch classes and qualities.
tests/baseline_images/test_display/test_display_chord_nolabels.png Adds the baseline image for the new chord display mpl test.
setup.cfg Raises minimum Python/numpy/matplotlib versions and updates classifiers/extras.
.github/workflows/test.yml Updates Python test matrix, records mpl results, and uploads mpl failure artifacts.
.github/environment.yml Updates conda env minimums (numpy/matplotlib) for CI.
.github/environment-minimal.yml Updates minimal dependency pins and adds a pillow pin for test compatibility.
.github/environment-docs.yml Updates docs environment minimums (numpy/matplotlib).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_display.py
Comment thread mir_eval/display.py
Comment thread mir_eval/display.py
Comment on lines +1331 to +1335
ymin=base,
ymax=height,
**seg_map[lab],
transform=transform,
)
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

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

Passing transform=transform into ax.axvspan() will set transform=None when base/height are provided, which can override the artist's default transform and produce incorrect placement (or identity transform). Consider omitting the transform kwarg when transform is None, or set it explicitly to the appropriate data transform when base/height are data coordinates.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This issue touches multiple segment plots, so I'd rather handle it in a dedicated PR.

I think the clean fix here is to make a second kwarg dict that either holds transform (if nontrivial) or is empty. This shouldn't change anything functionally in almost every case, but it would be just a smidge cleaner.

Comment thread .github/workflows/test.yml
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mir_eval/display.py Outdated
Comment thread .github/environment-minimal.yml Outdated
Comment thread mir_eval/display.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

setup.cfg:73

  • The conditional dependency syntax 'pillow == 9.0.0; numpy == 1.20.3' uses '==' for both package and environment marker. However, environment markers typically use comparison operators on package versions differently. The correct syntax should be 'pillow == 9.0.0; python_version == "3.8"' or check if pip's environment markers support version comparisons for dependencies. This syntax may not work as intended - pip environment markers check installed package versions, not dependency specifications.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mir_eval/display.py
Comment thread mir_eval/display.py
Comment thread .github/environment-minimal.yml Outdated
Comment thread mir_eval/display.py
Comment thread mir_eval/display.py
Comment thread tests/test_display.py
Comment thread mir_eval/display.py Outdated
bmcfee and others added 2 commits February 19, 2026 13:48
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@bmcfee
Copy link
Copy Markdown
Collaborator Author

bmcfee commented Feb 19, 2026

Doc build failure is due to a down crosslink out of our control, appears to be temporary. Otherwise I think we can stick a fork in this one?

@craffel
Copy link
Copy Markdown
Collaborator

craffel commented Feb 19, 2026

That's a bit of a mixed metaphor when we're talking about software development!

@craffel craffel merged commit 47bb42f into main Feb 19, 2026
11 of 12 checks passed
@bmcfee bmcfee deleted the chord-display branch February 19, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants