Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

display submodule #196

Merged
merged 21 commits into from
Jun 30, 2016
Merged

display submodule #196

merged 21 commits into from
Jun 30, 2016

Conversation

bmcfee
Copy link
Collaborator

@bmcfee bmcfee commented May 28, 2016

This implements #28

TODO:

  • segments
  • intervals
  • segment hierarchy
  • notes
  • contours
  • source separation
  • tests

@bmcfee bmcfee changed the title display submodule [WIP] display submodule May 28, 2016
@craffel craffel mentioned this pull request May 28, 2016
@craffel
Copy link
Collaborator

craffel commented May 28, 2016

How about segments and labeled_intervals?

I'm not crazy about this name because you can also use labels in segments. I am fine with segments and intervals. Segments meaning "I have a segmentation of time", intervals meaning "I have a bunch of intervals which might overlap". But also, it's a name, I'm happy with whatever.

I think it actually makes sense to drop property cycling entirely from the labeled-interval display.

Ok.

for the segment display, I think it makes more sense to rely on the axes internal fill style cycler, rather than roll our own.

Sounds good. Maybe we can include a small example of how to set an axis' color cycler? I bet many people have not done this. Or, whatever.

ehh.. y is ambiguous here, in that it can mean target or y-axis (in this case, kinda both).

What's target? label_set is fine.

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 28, 2016

But also, it's a name, I'm happy with whatever.

Yeah, I think if we provide examples showing the output of each, it will be obvious enough.

Maybe we can include a small example of how to set an axis' color cycler? I bet many people have not done this. Or, whatever.

I figure most people will just do this via plt.style.use('seaborn-muted') or somesuch. There's an example of more direct manipulation here, though that works on line styles and not fill styles. It shouldn't be hard to adapt, but I don't think it's a high priority.

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 30, 2016

Here's an example of hierarchical segment plots, comparing two annotations from salami:

plt.figure(figsize=(12, 8))
ax = plt.subplot(2,1,1)
mir_eval.display.labeled_intervals(int2, lab2, ax=ax, alpha=0.8, label='small')
mir_eval.display.labeled_intervals(int1, lab1, ax=ax, alpha=0.8, label='functions')
plt.legend(loc='best')
plt.title('Annotator 1')


ax = plt.subplot(2,1,2)
mir_eval.display.labeled_intervals(int4, lab4, ax=ax, alpha=0.8, label='small')
mir_eval.display.labeled_intervals(int3, lab3, ax=ax, alpha=0.8, label='functions')
plt.title('Annotator 2')
plt.legend(loc='best')

image

I think I generally prefer this to the analogous MSAF-style plot:

plt.figure(figsize=(12, 8))
ax = plt.subplot(2,1,1)
mir_eval.display.segments(int1, lab1, base=0.5, ax=ax, text=True)
mir_eval.display.segments(int2, lab2, height=0.5, ax=ax, text=True)
plt.title('Annotator 1')

ax = plt.subplot(2,1,2)
mir_eval.display.segments(int3, lab3, base=0.5, ax=ax, text=True)
mir_eval.display.segments(int4, lab4, height=0.5, ax=ax, text=True)
plt.title('Annotator 2')

image

The first one is a little easier to read (IMO) for a few reasons:

  1. no reuse of colors between segment levels
  2. color can be used to differentiate levels
  3. ability to set legend entries (small and functions)for different levels
  4. spatial patterns between levels are easier to discern than the color patterns in the bottom plot.

Anyone have thoughts for how the segment plot can be improved? @urinieto ?

@craffel
Copy link
Collaborator

craffel commented May 30, 2016

The only thing I like a bit more in the "analogous MSAF-style" is that it's visually easier for me to see which subsegments in the lower plot correspond to larger segments in the upper plot. The vertical separation in the first one makes this a little less obvious to me. This could be solved maybe by drawing dashed lines in the lower plot corresponding to boundaries in the upper plot, or something.

Playing devil's advocate, now:

no reuse of colors between segment levels

This is the worse part, IMO, of the "analogous MSAF-style", but could pretty easily be solved if you dive back into automatic color cycling.

color can be used to differentiate levels

Well, so does vertical position - I don't think it's hard to differentiate levels in the "analogous MSAF-style".

ability to set legend entries (small and functions)for different levels

These could be y labels.

spatial patterns between levels are easier to discern than the color patterns in the bottom plot.

I think if there was no color re-use it would be about as easy either way, assuming there were not too many colors.

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

The only thing I like a bit more in the "analogous MSAF-style" is that it's visually easier for me to see which subsegments in the lower plot correspond to larger segments in the upper plot. The vertical separation in the first one makes this a little less obvious to me. This could be solved maybe by drawing dashed lines in the lower plot corresponding to boundaries in the upper plot, or something.

Really? I find it pretty easy, but maybe that's because I know what to expect. We could add ticks and grid lines, but it would probably get cluttered quickly.

but could pretty easily be solved if you dive back into automatic color cycling.

Not really; you'd eventually have to rely on texture and alpha channels. Even then, the biggest complaint that I've gotten from people is that colors don't mean anything across hierarchy levels, but they really want them to. I think it's best to avoid that whole hornet's nest if we can.

Well, so does vertical position - I don't think it's hard to differentiate levels in the "analogous MSAF-style".

For two levels, no, but for deeper structures it can get messy.

These could be y labels.

Do you mean tick marks on the y axis? It's possible, sure.

I think if there was no color re-use it would be about as easy either way, assuming there were not too many colors.

I disagree, but that's anecdotal. Opinions from others would be useful here.

@craffel
Copy link
Collaborator

craffel commented May 31, 2016

Really? I find it pretty easy, but maybe that's because I know what to expect. We could add ticks and grid lines, but it would probably get cluttered quickly.

Yeah, I have to keep changing focus from top to bottom plot. It's not that bad.

I think it's best to avoid that whole hornet's nest if we can.

Fair enough.

For two levels, no, but for deeper structures it can get messy.

Wouldn't lower just mean finer structure regardless of the number of levels? Or is that not how hierarchical structure is defined?

Do you mean tick marks on the y axis?

Yep.

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

Pinging @justinsalamon re: pitch and notes.

I see in your prototype that you used rectangles for notes, and the y axis was set to a midi (numeric) scale. I think this can be plugged directly into labeled_intervals as is.

I think we can do a little better if we define a FuncFormatter that can reinterpret midi numbers as pitches (with octave and cent deviation) or frequencies (Hz). I'd prefer this to raw numbers or explicit ticker strings (like librosa currently does) because it will behave nicely with interactive plotting/zooming/etc.

For contours, we've talked about plotting on a cent axis. I think it makes the most sense to just map to continuous/fractional midi numbers via 12 * (log2(f) - log2(440)) + 69. That way you can overlay pitch contours on quantized midi notes.

It would also be nice to plot contours directly in frequency space for easy overlaying on spectrograms. Ultimately, this should be easy to do with a flag. The only nontrivial part of pitch contour plotting then will be voicing detection and grouping of samples into fragments.

Am I missing anything here?

@justinsalamon
Copy link
Collaborator

I see in your prototype that you used rectangles for notes, and the y axis was set to a midi (numeric) scale. I think this can be plugged directly into labeled_intervals as is.

hmmm, maybe? MIDI notes can but don't have to be quantized. If I understand correctly labeled intervals builds the y-axis labels from the data labels? This might not a good option for non-quantized pitch data?

For contours, we've talked about plotting on a cent axis. I think it makes the most sense to just map to continuous/fractional midi numbers via 12 * (log2(f) - log2(440)) + 69. That way you can overlay pitch contours on quantized midi notes.

👍

The only nontrivial part of pitch contour plotting then will be voicing detection and grouping of samples into fragments.

For melody this shouldn't be a problem since there's only (at most) one active pitch per timestamp. Voicing can be indicated by a change of style I suppose (color? line type? alpha?). Grouping samples into contours becomes a tough cooky for multi-f0, especially since the mir_eval format (correct me @rabitt if I'm wrong) doesn't specify which samples belong to which contour. Maybe apply basic TF-clustering as a best guess?

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

hmmm, maybe? MIDI notes can but don't have to be quantized. If I understand correctly labeled intervals builds the y-axis labels from the data labels? This might not a good option for non-quantized pitch data?

Yeah, fair point. We might have to refactor a bit to separate the patch logic from the vertical positioning. The bigger issue though is that we don't have a great way to specify the height of rectangles: should they span an entire semitone? That doesn't seem quite right. Maybe it would be better to just dump these into a line plot instead?

Voicing can be indicated by a change of style I suppose (color? line type? alpha?).

I was going to just not plot the unvoiced segments, but i suppose you might want those anyway. As a dirty hack you could simply to display.pitch(t, f) ; display.pitch(t, -f, alpha=0.5) for the same effect.

Grouping samples into contours becomes a tough cooky for multi-f0,

ugh... yeah, i forgot about multi-f0 plotting here. This seems like a problem that needs to be resolved at the multipitch API first, and display later. I really dislike the idea of clustering tf values in the display module to sort that out.

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

Quick proof of concept for pitch contour plotting:

ref_times, ref_freqs = mir_eval.io.load_labeled_events('/home/bmcfee/git/mir_eval/tests/data/melody/ref00.txt')
est_times, est_freqs = mir_eval.io.load_labeled_events('/home/bmcfee/git/mir_eval/tests/data/melody/est00.txt')

plt.figure(figsize=(12, 4))
mir_eval.display.pitch(ref_times, ref_freqs, unvoiced=True, midi=True,
                       linewidth=4, alpha=0.5, label='Reference')
mir_eval.display.pitch(est_times, est_freqs, unvoiced=True, midi=True, 
                       linewidth=1, alpha=1, label='Estimate')
plt.legend(loc='best')
plt.tight_layout()

image

It allows you to toggle plotting of unvoiced regions (which are depicted with the same style, but half the alpha as the voiced regions). A region is considered "voiced" if all of its frequencies are nonzero (and freq_to_voicing says so); this avoids underflow in the log scaling (midi units) conversion.

Styles are inherited from the axis line cycler, and the same style is applied to all fragments within a call to display.pitch.

@justinsalamon
Copy link
Collaborator

Yeah, fair point. We might have to refactor a bit to separate the patch logic from the vertical positioning. The bigger issue though is that we don't have a great way to specify the height of rectangles: should they span an entire semitone? That doesn't seem quite right. Maybe it would be better to just dump these into a line plot instead?

This is a matter of taste. My personal preference would be for each note to span a semitone (in height), since it matches the convention used in MIDI editors in DAWs so I find it intuitive. While it's true that a note doesn't inherently "span" a pitch range, I think the most common use case for this visualization will be for notes quantized on a semitone grid, for which this styling would be most helpful I think.

I was going to just not plot the unvoiced segments, but i suppose you might want those anyway. As a dirty hack you could simply to display.pitch(t, f) ; display.pitch(t, -f, alpha=0.5) for the same effect.

Yeah you definitely want to be able to display unvoiced pitch estimates. Not sure what your hack exapmle achieves?

ugh... yeah, i forgot about multi-f0 plotting here. This seems like a problem that needs to be resolved at the multipitch API first, and display later. I really dislike the idea of clustering tf values in the display module to sort that out.

Even if you sort it for the JAMS API, once you pass it to mir_eval it will be converted to the MIREX format which doesn't specify grouping. So as long as mir_eval supports the MIREX format, it's not clear to me how this can be solved on an API level?

@justinsalamon
Copy link
Collaborator

Quick proof of concept for pitch contour plotting:

This looks almost right to me:

  • I can't tell the difference between voiced and unvoiced segments. Maybe using a dashed line for unvoiced would be easier to see?
  • The colors used for ref and est make it very hard to distinguish them, especially when they overlap. I trust this can be changed though.

A region is considered "voiced" if all of its frequencies are nonzero (and freq_to_voicing says so)

What do you mean by this? Why do you need to infer if a region is voiced beyond what's already indicated by the raw data?

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

This looks almost right to me:

style choices are left as an exercise to the reader. also note: this is using mpl 2.0.0b1, which may have some kinks to work out.

What do you mean by this? Why do you need to infer if a region is voiced beyond what's already indicated by the raw data?

A region can be "unvoiced" with either negative or 0 frequency. If it's negative, it gets mapped to positive, and voicing is set to False. If it's 0, well, it stays 0. When you go to take a log for midi conversion, explosions happen. In general, I think you just don't want to show 0-frequency values at all, since they distort the axes and convey no information. Am I wrong here?

@justinsalamon
Copy link
Collaborator

In general, I think you just don't want to show 0-frequency values at all, since they distort the axes and convey no information. Am I wrong here?

Yes, that's correct. A 0 Hz estimate means "I detected no pitch here" and there's no reason to display it. A negative value means "I detected pitch here but I don't think it's part of the melody (but it might be if I'm wrong)", which is what we want to display as "unvoiced".

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

And with dynamic-friendly midi ticker conversion 🎶
mpl_midi

(check the lower-right for hover coordinates and the ticks on the left as we zoom in)

@justinsalamon
Copy link
Collaborator

Nice!

@bmcfee
Copy link
Collaborator Author

bmcfee commented May 31, 2016

And with the piano roll:

ref_times, ref_pitches = mir_eval.io.load_valued_intervals('/home/bmcfee/git/mir_eval/tests/data/transcription/ref03.txt')
est_times, est_pitches = mir_eval.io.load_valued_intervals('/home/bmcfee/git/mir_eval/tests/data/transcription/est03.txt')


plt.figure(figsize=(8, 4))

mir_eval.display.piano_roll(ref_times, ref_pitches, label='Reference', alpha=0.5)
mir_eval.display.piano_roll(est_times, est_pitches, label='Estimate', alpha=0.5, facecolor='r')

plt.ylim(ref_midis.min(), ref_midis.max())
plt.grid('on')
plt.legend(loc='best')
mir_eval.display.ticker_notes()
plt.tight_layout()

after some zooming in interactive mode:

image

@justinsalamon
Copy link
Collaborator

Nice! I'm confused about the y-axis gridlines: shouldn't notes be centered on the pitch values? It seems like right now they sit "on top" of the actual pitch value?

Another side issue is the density of the y-grid. For a piano roll I'd like to always have a y-tick (and label) for every semitone, independently of the zoom level of the plot. Otherwise it gets a little tricky to determine the pitch of some notes without counting with your fingers on the screen :)

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 1, 2016

Nice! I'm confused about the y-axis gridlines: shouldn't notes be centered on the pitch values? It seems like right now they sit "on top" of the actual pitch value?

They're baseline-aligned to the bars; the ticks/grid lines go below the relevant note, but the text labels are where they should be. I don't want to have them run through the rects because that would put the patch positions at non-integer values, and generally make life a pain. (If you don't believe me, try manipulating a bar plot some time.)

Another side issue is the density of the y-grid. For a piano roll I'd like to always have a y-tick (and label) for every semitone, independently of the zoom level of the plot. Otherwise it gets a little tricky to determine the pitch of some notes without counting with your fingers on the screen :)

Yup, just added minor ticks at all semitones for the piano roll plotter. Labels are still dynamically generated; if you want them in specific locations, that's your business as a discerning user with impeccable taste.

@justinsalamon
Copy link
Collaborator

They're baseline-aligned to the bars; the ticks/grid lines go below the relevant note, but the text labels are where they should be. I don't want to have them run through the rects because that would put the patch positions at non-integer values, and generally make life a pain. (If you don't believe me, try manipulating a bar plot some time.)

It might be easier to implement, but it's hella-confusing. The note above the D#6 label is a D#6? eek!

FWIW, this is how the big guys do it:
Image

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 1, 2016

It might be easier to implement, but it's hella-confusing.

hella you talkin about? That's the same thing: the label "C1" is parallel to the notes at pitch C1.

EDIT derp, my bad. The labeled_interval plots align the ticks correctly, but it got turned off in the screenshot i pasted before. Here's how it looks on my end currently:

image

@justinsalamon
Copy link
Collaborator

justinsalamon commented Jun 1, 2016

Not sure I follow. In the logic screenshot above, the label C1 is vertically aligned with notes representing a C1. In the piano roll figure, the labels are vertically aligned with the gridlines, which aren't aligned with any of the notes.

EDIT: this might be one of those things that's more easily hashed out in person with a whiteboard

EDIT: ahh... yes, now it makes sense 👍

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 1, 2016

Okay folks. I feel like the bulk of this, not counting source separation is basically done from a prototyping perspective. Here's a notebook that you can play with, feedback is welcome.

At this point, it's worth thinking about how we should test things. Matplotlib-style image comparison regression tests? They're kind of a pain to set up, but I'm not sure what else we can do.

@craffel
Copy link
Collaborator

craffel commented Jun 1, 2016

Thanks @bmcfee. I am currently moving everything out of my house into a big box so won't get a chance to look at this for a day or two. One thing that popped up and I don't think was resolved:

The only nontrivial part of pitch contour plotting then will be voicing detection and grouping of samples into fragments.

(and the ensuing discussion). I think the only sane thing to do is to not group at all, and just plot multi-f0 as non-connected dots. That's how they're evaluated, anyways (right?). I definitely do not think TF clustering is something we want to reckon with, unless there is a very simple approach which does the right thing the vast majority of the time.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 20, 2016

Maybe it would be nice to add a ticker for x-axis in beats and bars?

Seems like a nice idea, but it seems a little outside of mir_eval's style as written above. Inferring beat position from (static) tempo doesn't seem precise enough to me -- I'd rather have a way to visualize known/estimated beat/downbeat positions.

We already have event viz for beats/downbeats, but they display as lines instead of tickers. I think this is reasonable because you can also read off the time of the beats instead of just metrical position, which should be relatively obvious if you plot downbeats as well.

@craffel
Copy link
Collaborator

craffel commented Jun 25, 2016

Ok, spent some time playing with this. It's great. I had a lot of fun visualizing some pretty_midi stuff. Go MIDI! Here are some thoughts/questions/observations, some of which I think can be safely ignored, but were things that came up as I was messing around.

  • I would expect the display functions to automatically make x and y axis labels (like in librosa).
  • mir_eval.display.events(events, base=1., height=2.) displays nothing when using a new figure, because it does not change the boundaries from [0, 1], [0, 1] (same with other functions where you can change the plot range)
  • mir_eval.display.events(events, colors='k') doesn't change the line color to black (but plt.vlines(events, colors='k') does).
  • mir_eval.display.events(np.linspace(0, 10, 5) sets the xlim to [0, 10], which makes the first and last lines invisible (a Matplotlib policy, I think). I feel like it would be slick if it adjusted the range to always be slightly larger than the range of the events, for this reason.
  • So what exactly does the levels kwarg do in hierarchy? It doesn't seem to change the figure.
  • Sort of similar to above - mir_eval.display.labeled_intervals(intervals, labels, tick=False) on a new graph keeps the y limits 0-1.
  • If, someday, someone solves TF clustering for multipitch, that would be cool.
  • mir_eval.display.labeled_intervals(np.zeros((0, 2)), np.zeros((0,))) gives a ValueError. I think it would be a little more graceful if it just didn't plot anything, like plt.plot([]). Same for the others - error shows up in __expand_limits.
  • In mir_eval.display.separation's docstring, sources is specified as a list of np.ndarray. I think we should enforce that it be a np.ndarray of shape (n_src, n_sample), because if it's a ragged list, the function will puke.
  • I wish that the output of separation was not quite so dependent on the order in which the sources are supplied. Do you have any idea as to how we could adjust the threshold in terms of what gets turned transparent in each spectrogram (if that makes any sense)? Here's an example of three sources from a MIDI file, the piano roll:

piano_roll

mir_eval.display.separation(sources)

sources

mir_eval.display.separation([sources[2], sources[1], sources[0]])

sources_2

  • It is a little funky to me that only separation uses a legend, and also creates it automatically, whereas as fara s I can tell the other functions don't have a mechanism to create a legend at all. Why is this?

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 25, 2016

I would expect the display functions to automatically make x and y axis labels (like in librosa).

In retrospect I think this is a bad convention.

mir_eval.display.events(events, base=1., height=2.) displays nothing when using a new figure, because it does not change the boundaries from [0, 1], [0, 1](same with other functions where you can change the plot range)

  • Good call. We can fix that.

mir_eval.display.events(events, colors='k') doesn't change the line color to black (but plt.vlines(events, colors='k') does).

shouldn't it be color='k'?

mir_eval.display.events(np.linspace(0, 10, 5) sets the xlim to [0, 10], which makes the first and last lines invisible (a Matplotlib policy, I think). I feel like it would be slick if it adjusted the range to always be slightly larger than the range of the events, for this reason.

That should be outside the scope of mir_eval; there's an open pr/discussion on matplotlib to do more intelligent automatic range limiting.

So what exactly does the levels kwarg do in hierarchy? It doesn't seem to change the figure.

It's in the docstring. If you put a legend on a hierarchy plot, levels[i] is the legend entry for the ith level. This could be more clear.

Sort of similar to above - mir_eval.display.labeled_intervals(intervals, labels, tick=False) on a new graph keeps the y limits 0-1.

What would you expect it to do?

If, someday, someone solves TF clustering for multipitch, that would be cool.

I still think we shouldn't do that; instead, accept an input data structure that indexes tf values by contour id.

ir_eval.display.labeled_intervals(np.zeros((0, 2)), np.zeros((0,))) gives a ValueError. I think it would be a little more graceful if it just didn't plot anything, like plt.plot([]). Same for the others - error shows up in __expand_limits.

  • Good catch; will fix

In mir_eval.display.separation's docstring, sources is specified as a list of np.ndarray. I think we should enforce that it be a np.ndarray of shape (n_src, n_sample), because if it's a ragged list, the function will puke.

I was going for consistency with the separation metrics, but sure.

I wish that the output of separation was not quite so dependent on the order in which the sources are supplied. Do you have any idea as to how we could adjust the threshold in terms of what gets turned transparent in each spectrogram (if that makes any sense)? Here's an example of three sources from a MIDI file, the piano roll:

I tried a whole lot to work that out, but nothing seemed to do what I wanted. Maybe it's possible if you render each spectrogram as RGBA image first, and then average in RGBA space.

@craffel
Copy link
Collaborator

craffel commented Jun 26, 2016

In retrospect I think this is a bad convention.

Haha, I could see why you'd say that. Fair enough.

shouldn't it be color='k'?

From vlines' docstring: "colors : array_like of colors, optional, default: 'k'" So, either should work. I'm not sure why colors isn't working in events.

What would you expect it to do?

It's the same issue as above:

mir_eval.display.labeled_intervals(intervals, labels)

labeled_intervals

mir_eval.display.labeled_intervals(intervals, labels, tick=False)

labeled_intervals_tick_false

I still think we shouldn't do that; instead, accept an input data structure that indexes tf values by contour id.

Yeah, or provide a kwarg which does that, or something. But we don't need to worry about that now.

I was going for consistency with the separation metrics, but sure.

I think they all specify e.g. reference_sources : np.ndarray, shape=(nsrc, nsampl) like this.

I tried a whole lot to work that out, but nothing seemed to do what I wanted. Maybe it's possible if you render each spectrogram as RGBA image first, and then average in RGBA space.

It seems (from the point of view of an outsider who has no idea what's going on) that, under some threshold, alpha is set to 0. Is there any easy way of modifying that threshold, so that more of the spectrogram blobs get made transparent?

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 27, 2016

.events(events, base=1., height=2.) displays nothing when using a new figure, because it does not change the boundaries

Can you give me an example that demonstrates this behavior? I'm not sure what you mean by "new figure" (as opposed to new axes); when I do:

fig = plt.gcf()
mir_eval.display.events(events, figure=fig)

it works as expected. Are you doing something differently?

From vlines' docstring: "colors : array_like of colors, optional, default: 'k'" So, either should work. I'm not sure why colors isn't working in events.

I think what's happening here is that color (popped from the cycler) supersedes colors; either will work in vlines, but color takes precedence. This seems like a (documentation) error in matplotlib; i'm inclined to leave it as is, since it would otherwise take some pretty ugly contortions to work around with the axis cycler.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 27, 2016

Is there any easy way of modifying that threshold, so that more of the spectrogram blobs get made transparent?

Not really. The colormaps are generated by linear interpolation; what you're describing would require a nonlinear interpolation, and exposing that in a clean way to the user is non-trivial.

I also looked into making the maximum alpha value tuneable. This is easy in mpl 2.0 via mpl.colors.to_rgba(), but in 1.5 it doesn't quite work. (The difference comes in what kinds of inputs to_rgba supports in each version.) We can revisit this in the future, but i think for now we should keep it as is.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 27, 2016

It is a little funky to me that only separation uses a legend, and also creates it automatically, whereas as fara s I can tell the other functions don't have a mechanism to create a legend at all. Why is this?

Yeah, it sucks. Basically, there's no way to attach legend entries (labels) to colormeshes, so we have to make proxy artists and label those instead. To make that work, we need to first construct a legend, and there's no way to do that without drawing the legend. (There might be a way to do it by making 0px-by-0px patches attached to the axes, but that seems ... perilous.)

At any rate, it doesn't bother me too much since the separation plots are completely uninterpretable without a legend, whereas the remaining plots may or may not be.

@craffel
Copy link
Collaborator

craffel commented Jun 27, 2016

Can you give me an example that demonstrates this behavior?

mir_eval.display.events(np.arange(10))

events

mir_eval.display.events(np.arange(10), base=1, height=2)

events_base_height

I think what's happening here is that color (popped from the cycler) supersedes colors; either will work in vlines, but color takes precedence.

That makes sense. The colors argument is useful in its own right though (allows for an array-like of len equal to x to be passed), so it may be confusing to end-users if it doesn't work with events. Not a big deal though.

We can revisit this in the future, but i think for now we should keep it as is.

Ok.

Yeah, it sucks. Basically, there's no way to attach legend entries (labels) to colormeshes, so we have to make proxy artists and label those instead. To make that work, we need to first construct a legend, and there's no way to do that without drawing the legend.

Can we call ax.legend().set_visible(False) or something, and have things work the way we want them to?

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 27, 2016

Can we call ax.legend().set_visible(False) or something, and have things work the way we want them to?

Apparently not. You can hide the legend that way, but then drawing it again doesn't work. At best, it would have to be something like:

ax = mir_eval.display.separation(some garbage)
ax.get_legend().set_visible(true)

instead of ax.legend(). I'll take another stab at attaching proxies to the axis, but if it doesn't work, i don't see much of a problem in keeping it as is.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 27, 2016

Ok, got it working with 0x0 rect patches. It's a brutal hack, but it works how you describe, and as an added bonus, makes it easier to manipulate the legend options.

Apart from the transparency thing in separation, I think I've hit all of @craffel's points.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 27, 2016

Okay, added a maximum alpha value for source separation display. It doesn't achieve everything we'd want, but it does make it so that sources with comparable amplitudes (near max) don't mask each other out.

@craffel
Copy link
Collaborator

craffel commented Jun 28, 2016

Okay, added a maximum alpha value for source separation display. It doesn't achieve everything we'd want, but it does make it so that sources with comparable amplitudes (near max) don't mask each other out.

Nice. This and the legend looks nice.

Final nitpick: Setting events' base and height kwargs looks different than setting vlines equivalent ymin and ymax kwargs.. If you have a good reason it should be this way, that's ok, but it hurts my sensibilities a little bit.

plt.vlines(np.arange(10), 10, 12)

vlines

mir_eval.display.events(np.arange(10), base=10, height=2)

events

@@ -346,6 +346,7 @@ def test_separation_label():

plt.legend()


Copy link
Contributor

Choose a reason for hiding this comment

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

image

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 28, 2016

Final nitpick: Setting events' base and height kwargs looks different than setting vlines equivalent ymin and ymax kwargs.. If you have a good reason it should be this way, that's ok, but it hurts my sensibilities a little bit.

It's done that way for consistency with labeled_intervals and segments.

@craffel
Copy link
Collaborator

craffel commented Jun 28, 2016 via email

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 28, 2016

Sure, I'm just used to matplotlib fitting the axis limits to the content,
but we can do something different.

Oh, I misunderstood -- I thought you were talking about the API.

I fixed the limit detection logic to do the right thing, but the API is designed for consistency across functions.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jun 28, 2016

@craffel btw, this should be ready to go. If you want to run that last example through one more time, please do so before I merge.

@craffel craffel merged commit 5887679 into master Jun 30, 2016
@craffel
Copy link
Collaborator

craffel commented Jun 30, 2016

Merged, thank you again!

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jul 1, 2016

👍 💯

@bmcfee bmcfee deleted the display branch July 1, 2016 15:02
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.

5 participants