marl / jams

A JSON Annotated Music Specification for Reproducible MIR Research
ISC License
186 stars 27 forks source link

Display via mir_eval #115

Closed bmcfee closed 8 years ago

bmcfee commented 8 years ago

For when mir_eval's display module gets merged.


This change is Reviewable

bmcfee commented 8 years ago

Anyone have opinions about how annotation metadata could/should be included in the generated figures? I was thinking about making a separate subplot joined to the right edge of the main subplot, and populating that with a text box. Maybe there's a nicer way?

I'd rather not clutter the main figure too much.

bmcfee commented 8 years ago

Here's what it looks like now:

fig, _ = jams.display.display_multi(jam.annotations[:3], fig_kw=dict(figsize=(8, 4)))
fig.tight_layout()

image

urinieto commented 8 years ago

I will quickly chime in to a simply say that I think this is awesome.

On Tue, Jun 14, 2016 at 6:51 PM, Brian McFee notifications@github.com wrote:

Here's what it looks like now:

fig, _ = jams.display.display_multi(jam.annotations[:3], fig_kw=dict(figsize=(8, 4))) fig.tight_layout()

[image: image] https://cloud.githubusercontent.com/assets/1190540/16065578/16760854-327a-11e6-8eec-86000654e3f6.png

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/marl/jams/pull/115#issuecomment-226067173, or mute the thread https://github.com/notifications/unsubscribe/ADhisd_UA8PEupBGVhJa8CZ3BgL9pAxjks5qL1q0gaJpZM4It4BU .

bmcfee commented 8 years ago

This one is now up to date with the pitch namespace refactor.

note_* namespaces will now render as piano rolls. pitch_contour renders contours grouped into colors by id. If you want a single color for all contours in a plot, you can clobber that by saying jams.display.display(jam.annotation[i], color='b') or whatever.

I think this is ready for some serious tire-kicking. I guess @rabitt and @justinsalamon have the most data that would be useful for testing here.. little help?

bmcfee commented 8 years ago

Test coverage is now up to par.

It depends on mir_eval 0.4 though, so we should wait to merge until that's released.

stefan-balke commented 8 years ago

@bmcfee there is a missing from . import display

int the __init__.py

stefan-balke commented 8 years ago

Works, but looks different here. screen shot 2016-07-28 at 17 01 47

bmcfee commented 8 years ago

No there isn't.

mir_eval decided to have matplotlib as an optional dependency, so the display module cannot be loaded automatically at the top-level. JAMS could resolve this by adding an mpl dependency, but given the strange issues around leaking file descriptors and the potential use of JAMS on server-side applications, we decided to keep the optional import.

bmcfee commented 8 years ago

looks different here.

What's your mpl version?

stefan-balke commented 8 years ago

What's your mpl version?

matplotlib (1.5.2) on OS X

stefan-balke commented 8 years ago

mir_eval decided to have matplotlib as an optional dependency, so the display module cannot be loaded automatically at the top-level. JAMS could resolve this by adding an mpl dependency, but given the strange issues around leaking file descriptors and the potential use of JAMS on server-side applications, we decided to keep the optional import.

Got it, thanks.

stefan-balke commented 8 years ago

Could be a backend problem, couldn't it? Or do you have some fancy settings in your mpl-rc?

BTW: If you do not use fig.tight_layout(), this will happen.

bennycarter_sweetlorraine

bmcfee commented 8 years ago

It's almost certainly a theme issue. I'm not worrying about that for now because mpl 2 will change all the defaults anyway.

If you want to reproduce the examples I pasted, you might try

import matplotlib.style
matplotlib.style.use('seaborn-white')

(i think that's what i used, anyway.)

stefan-balke commented 8 years ago

Have this now:

import jams.display
import seaborn
import matplotlib.style
matplotlib.style.use('seaborn-white')

That worked. Thanks. chetbaker_longagoandfaraway

ejhumphrey commented 8 years ago
:lgtm:

_Comments from Reviewable_

ejhumphrey commented 8 years ago

@bmcfee I'll let you do the honors of the mergy


_Comments from Reviewable_

ejhumphrey commented 8 years ago

Reviewed 2 of 5 files at r1, 4 of 4 files at r4, 2 of 2 files at r5. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

bmcfee commented 8 years ago

I'll wait on merge until mir_eval releases so I can fix the version in the travis config.

bmcfee commented 8 years ago

mir_eval 0.4 is out; i'd like to merge this one today, once all the tests pass.

bmcfee commented 8 years ago

Reviewed 2 of 2 files at r6. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable