Sonify #91
Sonify #91
Conversation
|
Note: pitch sonification is broken because mir_eval doesn't support overlapping intervals (or explicit durations). |
|
Question for the crew: do we want to stall this until we can arrive at a good solution for pitch sonification? Or just merge as is? If the latter, can someone please CR this? (Looking at you, @justinsalamon :)) It shouldn't take long, as it's mainly just glue code that maps namespaces to mir_eval functions. |
|
|
||
| freqs = 440.0 * (2.0 ** ((np.arange(128) - 69.0)/12.0)) | ||
|
|
||
| gram = np.zeros((len(freqs), len(notes))) |
There was a problem hiding this comment.
this assumes non-overlapping notes, will break for overlapping notes
|
@bmcfee I wouldn't merge as is. If we remove the pitch_midi method and the mapping from SONIFY_MAPPING we could merge the rest, which seems fine to me. |
|
Based on how the |
That was my feeling as well. Alternately, if someone wants to PR mir_eval to fix the pitch rendering, we could just stall merging this one until the next revision. @craffel thoughts? |
|
What exactly is your gripe with |
Essentially that intervals are implicit, rather than explicit, so you can't easily specify duration. I can do the PR if nobody else wants to. |
|
I'm not sure what you mean, so maybe it'd be better if you did it. |
|
Am I wrong in understanding that |
It should work fine either way. |
ok.
I think it's more that time_frequency is meant for sonifying a single pitch trajectory, since the duration of one frequency is implicitly coded by the start time of the next. It's pretty trivial to fix this so that it accepts a [start, end] array instead of onset times, and does the right thing with overlap and add. |
Can you do it without breaking current functionality? We can discuss in the hypothetical PR. |
|
Tests are done and this should be ready to merge as soon as the next mir_eval release is out. |
bmcfee
force-pushed
the
sonify
branch
2 times, most recently
from
3ff860c to
450d0dd
Compare
|
@justinsalamon care to sign off on this one as well? |
|
@justinsalamon I'll trade you a mir_eval CR for a jams CR here.... |
|
@bmcfee sounds like a fair trade :) I'll give it a look this afternoon |
| **kwargs) | ||
|
|
||
|
|
||
| def pitch_hz(annotation, sr=22050, length=None, **kwargs): |
There was a problem hiding this comment.
The following code causes will cause a crash, though I think the problem is with the mir_eval code (this line), not the jams wrapper.
jam = jams.load('/Users/justin/dev/jams-data/datasets/MIREX05/train05REF.jams')
ann = jam.annotations[0]
jams.sonify.pitch_hz(ann)There's a division by the frequency value, but since the convention is to indicate silence (no pitch) by non-positive value, it breaks. Error:
---------------------------------------------------------------------------
ZeroDivisionError Traceback (most recent call last)
<ipython-input-9-ceb7518fe118> in <module>()
----> 1 jams.sonify.pitch_hz(ann)
/Users/justin/Documents/dev/jams/jams/sonify.pyc in pitch_hz(annotation, sr, length, **kwargs)
61 gram, freqs, intervals,
62 fs=sr, length=length,
---> 63 **kwargs)
64
65
/Users/justin/Documents/dev/mir_eval/mir_eval/util.pyc in filter_kwargs(_function, *args, **kwargs)
741 filtered_kwargs[kwarg] = value
742 # Call the function with the supplied args and the filtered kwarg dict
--> 743 return _function(*args, **filtered_kwargs)
744
745
/Users/justin/Documents/dev/mir_eval/mir_eval/sonify.pyc in time_frequency(gram, frequencies, times, fs, function, length)
127 for n, frequency in enumerate(frequencies):
128 # Get a waveform of length samples at this frequency
--> 129 wave = _fast_synthesize(frequency)
130 # Scale each time interval by the piano roll magnitude
131 for m, (start, end) in enumerate((times * fs).astype(int)):
/Users/justin/Documents/dev/mir_eval/mir_eval/sonify.pyc in _fast_synthesize(frequency)
107
108 # Generate ten periods at this frequency
--> 109 n_samples = int(10.0 * fs / frequency)
110
111 short_signal = function(2.0 * np.pi * np.arange(n_samples) *
ZeroDivisionError: float division by zero
There was a problem hiding this comment.
Ugh.
I guess this should be fixed within jams, not mir_eval, since mir_eval doesn't claim to support negative/zero-f inputs.
Since I don't have your data handy, can you paste a from-scratch example that triggers this?
There was a problem hiding this comment.
I guess this should be fixed within jams, not mir_eval, since mir_eval doesn't claim to support negative/zero-f inputs.
By "support" do you mean in general or the sonify module? mir_eval does support negative/zero frequency input for evaluation (e.g. the melody module).
Since I don't have your data handy, can you paste a from-scratch example that triggers this?
You do, it's in the jams-data repo ;)
There was a problem hiding this comment.
By "support" do you mean in general or the sonify module?
In sonify.
You do, it's in the jams-data repo ;)
What I meant was: I'm lazy and don't want to clone an entire repo just to cook up what should be a one-liner test case. 😞
No no, you can't have pitch and note as two separate things... a note is a representation of pitch! The options I'm proposing are either
That sounds fine to me, as long as we eventually kill |
|
Circling back on this, since #111 suggests that we're going to deprecate I think the piano roll sonification that we currently have will work fine for Implementing it within jams seems a little to special-case to me, and borders on bloat. What do y'all think @justinsalamon ? |
|
I guess it a philosophical question: where should sonification and visualization code live - mir_eval or JAMS? If you ask me I actually think both should be part of JAMS, since it's designed for annotation generation/manipulation/exploration, whereas mir_eval is focused on annotation comparison. Of course one could argue that evaluation by audition/visualization is relevant to mir_eval. I can also imagine other valid arguments for including this functionality in mir_eval, an obvious one being that if everything lives in JAMS people using mir_eval with different annotation formats can't take advantage of these tools. For historical reasons right now visualization lives inside JAMS and sonification lives inside mir_eval, though I think that's pretty arbitrary. |
That's why it was added.
This is important. Whatever the visualization/sonification tool is, it should (IMO) be storage format-independent.
Visualization was always intended to be part of |
Yes, but that was before JAMS was a thing (I think). Now you have 2 separate libraries, one for annotation wrangling and another for annotation comparison. Both visualization and sonification fit naturally into either. Come to think of it I'm actually not sure whether viz was always part of the plan for mir_eval: I actually wrote basic viz for melody extraction eval into the original mir_eval melody module but it was canned. |
|
It's been part of the plan for 2 years, at least: mir-evaluation/mir_eval#28 |
Haha ok cool. Still, we need to reach consensus as to where sonification and visualization go. I don't think it makes sense to have either spread across both libraries. |
In both cases, I think it makes sense to have core routines in mir_eval that do the heavy lifting, and wrappers in jams that handle conversion and maybe metadata decoration (in the case of viz). For the purposes of this discussion though, @craffel : how do you feel about adding a continuous pitch sonification routine alongside piano roll and clicks? It would make our lives easier over here. |
Well, like I said, as long as the visualizers are not tied to a specific data storage format, it's fine either way.
This seems to make sense to me.
Sure, that would be great to have. Realistically I won't do it anytime in the foreseeable future. |
understood; I'll try to find time for it soonish. |
|
Note to self: this PR is stalled by the following:
|
|
Holding off on moving this guy forward until after display merges. There's redundant changes there for ns conversion on the new pitch namespaces that we'll need here. |
|
Okay, I think this one's good to go now. NB: the semantics for pitch are only, say, 99% obvious. We support two styles of pitch sonification: piano-roll and contour. Piano roll has higher priority, so if you have an annotation in the old pitch contour namespace(s), it will be sonified by piano roll. This is because all pitch annotations can be coerced into a contour set, but not all can be coerced into a piano roll (eg, Practically, this means that only annotations of type |
|
Reviewed 1 of 4 files at r6. Comments from Reviewable |
|
This is basically done, modulo two things:
We could do this, but because of how mir_eval.sonify.clicks works, we'd have to synthesize our own click samples for different frequencies. Is this worth the effort? |
|
downbeats ( I think this covers everything except for the ones that don't make sense to sonify: Anyone care for a final CR? |
updated sonify module added pitch use mir_eval kwarg filtering on sonify functions added pitch_hz sonifier adding unit tests for sonify added ns-specific tests for sonify
fixing bugs in sonification orderddict for sonify_mapping, handle non-positive pitches added empty signal handling in pitch_hz simplified pitch_hz logic improved test coverage on pitch_hz sonify fixed instantaneous pitch contour generation fixed a broken test case in pitch_hz pitch_hz sonification via mir_eval pitch_contour
fixed unit tests for piano roll sonification added a contour sonification test fixed coveralls fixed multi-contour sonification fixed a basis error in multi-contour plotting added downbeat sonification modified downbeat metronome pitches fixed a length issue in pitch contour sonification expanded contour test
|
Reviewed 1 of 2 files at r1, 1 of 4 files at r6, 2 of 2 files at r7. Comments from Reviewable |
Implements #82
This change is