Sonify #91

Merged
merged 4 commits into from Sep 2, 2016

Projects

None yet

3 participants

@bmcfee
Contributor
bmcfee commented Dec 13, 2015 edited

Implements #82


This change is Reviewable

@bmcfee bmcfee added the enhancement label Dec 13, 2015
@bmcfee bmcfee self-assigned this Dec 13, 2015
@bmcfee bmcfee added this to the 0.2.2 milestone Dec 13, 2015
@bmcfee
Contributor
bmcfee commented Dec 13, 2015

Note: pitch sonification is broken because mir_eval doesn't support overlapping intervals (or explicit durations).

@bmcfee
Contributor
bmcfee commented Jan 30, 2016

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.

@justinsalamon justinsalamon and 1 other commented on an outdated diff Feb 3, 2016
jams/sonify.py
+def chord(annotation, sr=22050, length=None, **kwargs):
+ '''Sonify chords'''
+
+ intervals, chords = annotation.data.to_interval_values()
+
+ return mir_eval.sonify.chords(chords, intervals, fs=sr, length=length, **kwargs)
+
+
+def pitch_midi(annotation, sr=22050, length=None, **kwargs):
+ '''Sonify midi pitches'''
+
+ intervals, notes = annotation.data.to_interval_values()
+
+ freqs = 440.0 * (2.0 ** ((np.arange(128) - 69.0)/12.0))
+
+ gram = np.zeros((len(freqs), len(notes)))
@justinsalamon
justinsalamon Feb 3, 2016 Contributor

this assumes non-overlapping notes, will break for overlapping notes

@bmcfee
bmcfee Apr 26, 2016 Contributor

!resolved

@justinsalamon justinsalamon commented on an outdated diff Feb 3, 2016
jams/sonify.py
+ return mir_eval.sonify.chords(chords, intervals, fs=sr, length=length, **kwargs)
+
+
+def pitch_midi(annotation, sr=22050, length=None, **kwargs):
+ '''Sonify midi pitches'''
+
+ intervals, notes = annotation.data.to_interval_values()
+
+ freqs = 440.0 * (2.0 ** ((np.arange(128) - 69.0)/12.0))
+
+ gram = np.zeros((len(freqs), len(notes)))
+
+ for t, n in enumerate(notes):
+ gram[n, t] = 1.0
+
+ return mir_eval.sonify.time_frequency(gram, freqs, intervals[:, 0],
@justinsalamon
justinsalamon Feb 3, 2016 Contributor

note start times will be correct but durations will be set arbitrarily based on start time of next note

@justinsalamon
Contributor

@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.

@justinsalamon
Contributor

Based on how the time_frequency sonification method is written in mir_eval, a solution for sonifying pitch_midi would be to generate a gram with a higher temporal resolution and fill in activations for each note based its start time and duration. Could perhaps try and avoid unnecessary sparsity by rounding all start/end times to match a manageable hop size (e.g. 10ms?)

@bmcfee
Contributor
bmcfee commented Feb 3, 2016

@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.

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?

@craffel
craffel commented Feb 3, 2016

What exactly is your gripe with time_frequency? Can you make a PR?

@bmcfee
Contributor
bmcfee commented Feb 3, 2016

What exactly is your gripe with time_frequency? Can you make a PR?

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.

@craffel
craffel commented Feb 3, 2016

I'm not sure what you mean, so maybe it'd be better if you did it.

@justinsalamon
Contributor

Am I wrong in understanding that time_frequency in mir_eval was written with spectrograms in mind, not symbolic representations? Perhaps it's just not the right target method for sonifying MIDI-style note sequences?

@craffel
craffel commented Feb 3, 2016

Am I wrong in understanding that time_frequency in mir_eval was written with spectrograms in mind, not symbolic representations? Perhaps it's just not the right target method for sonifying MIDI-style note sequences?

It should work fine either way.

@bmcfee
Contributor
bmcfee commented Feb 3, 2016

I'm not sure what you mean, so maybe it'd be better if you did it.

ok.

Am I wrong in understanding that time_frequency in mir_eval was written with spectrograms in mind, not symbolic representations? Perhaps it's just not the right target method for sonifying MIDI-style note sequences?

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.

@craffel
craffel commented Feb 3, 2016

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.

@bmcfee
Contributor
bmcfee commented Feb 11, 2016

Tests are done and this should be ready to merge as soon as the next mir_eval release is out.

@bmcfee
Contributor
bmcfee commented Mar 1, 2016

@justinsalamon care to sign off on this one as well?

@bmcfee
Contributor
bmcfee commented Apr 21, 2016

@justinsalamon I'll trade you a mir_eval CR for a jams CR here....

@justinsalamon
Contributor

@bmcfee sounds like a fair trade :) I'll give it a look this afternoon

@justinsalamon justinsalamon and 1 other commented on an outdated diff Apr 25, 2016
jams/sonify.py
+ return filter_kwargs(mir_eval.sonify.clicks, interval[:, 0],
+ fs=sr, length=length, **kwargs)
+
+
+def chord(annotation, sr=22050, length=None, **kwargs):
+ '''Sonify chords'''
+
+ intervals, chords = annotation.data.to_interval_values()
+
+ return filter_kwargs(mir_eval.sonify.chords,
+ chords, intervals,
+ fs=sr, length=length,
+ **kwargs)
+
+
+def pitch_hz(annotation, sr=22050, length=None, **kwargs):
@justinsalamon
justinsalamon Apr 25, 2016 Contributor

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
@bmcfee
bmcfee Apr 25, 2016 Contributor

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?

@justinsalamon
justinsalamon Apr 25, 2016 Contributor

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 ;)

@bmcfee
bmcfee Apr 26, 2016 Contributor

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. 😞

@justinsalamon justinsalamon and 1 other commented on an outdated diff Apr 25, 2016
jams/sonify.py
+ **kwargs)
+
+
+def pitch_hz(annotation, sr=22050, length=None, **kwargs):
+ '''Sonify pitches in Hz'''
+
+ intervals, pitches = annotation.data.to_interval_values()
+
+ # Collapse down to a unique set of frequency values
+ freqs = np.unique(pitches)
+
+ # Build the piano roll
+ pitch_index = {p: i for i, p in enumerate(freqs)}
+ gram = np.zeros((len(freqs), len(pitches)))
+ for t, n in enumerate(pitches):
+ gram[pitch_index[n], t] = 1.0
@justinsalamon
justinsalamon Apr 25, 2016 Contributor

This is not incorrect, but ignores the fact that pitches can contain negative/zero frequencies, which will result in a gram that breaks themir_eval code.

@bmcfee
bmcfee Apr 25, 2016 Contributor

Have I mentioned that I hate this negative frequency convention?

@justinsalamon
justinsalamon Apr 25, 2016 edited Contributor

Have I mentioned that I hate this negative frequency convention?

Life sucks :P

Also, try sonifying this pitch_midi annotation (which gets coerced to a pitch_hz annotation by the current code), something's going wrong somewhere (I get really low-frequency sounds and a whole bunch of clicks):

{
  "sandbox": {}, 
  "annotations": [
    {
      "namespace": "pitch_midi", 
      "sandbox": {}, 
      "time": 0, 
      "duration": null, 
      "annotation_metadata": {
        "annotation_tools": "", 
        "curator": {
          "name": "", 
          "email": ""
        }, 
        "annotator": {}, 
        "version": "", 
        "corpus": "", 
        "annotation_rules": "", 
        "validation": "", 
        "data_source": ""
      }, 
      "data": {
        "duration": [
          0.28985500000000003, 
          0.262681, 
          0.108695, 
          0.16304300000000002, 
          0.28985500000000003, 
          0.271739, 
          0.28985500000000003, 
          0.28985500000000003, 
          0.182975
        ], 
        "confidence": [
          1.0, 
          1.0, 
          1.0, 
          1.0, 
          1.0, 
          1.0, 
          1.0, 
          1.0, 
          1.0
        ], 
        "value": [
          63.0, 
          62.0, 
          62.0, 
          60.0, 
          57.0, 
          56.0, 
          60.0, 
          63.0, 
          62.0
        ], 
        "time": [
          0.20833300000000002, 
          0.498188, 
          0.760869, 
          0.869564, 
          1.0326060000000001, 
          1.947461, 
          2.2282580000000003, 
          2.5271700000000004, 
          2.817025
        ]
      }
    }
  ], 
  "file_metadata": {
    "jams_version": "0.2.1", 
    "title": "TRACBNP128F427824_62450a93473b0b0d417420081945652b", 
    "identifiers": {}, 
    "release": "", 
    "duration": 3, 
    "artist": ""
  }
}
@bmcfee
bmcfee Apr 26, 2016 Contributor

!resolved

@justinsalamon justinsalamon and 1 other commented on an outdated diff Apr 25, 2016
jams/sonify.py
+ Raises
+ ------
+ NamespaceError
+ If the annotation has an un-sonifiable namespace
+ '''
+
+ length = None
+ if duration is not None:
+ length = int(duration * sr)
+
+ for namespace, func in six.iteritems(SONIFY_MAPPING):
+ try:
+ coerce_annotation(annotation, namespace)
+ return func(annotation, sr=sr, length=length, **kwargs)
+ except NamespaceError:
+ pass
@justinsalamon
justinsalamon Apr 25, 2016 Contributor

So you're choosing the sonification function by checking whether the annotation can be converted to each supported namespace and if so you sonify it? What if an annotation can be converted to two different namespaces, both of which have supported sonifications? Wouldn't this call both sonification functions?

Wouldn't it be cleaner to just check if the annotation namespace is a key in SONIFY_MAPPING and if so call the function it maps to?

@bmcfee
bmcfee Apr 25, 2016 Contributor

What if an annotation can be converted to two different namespaces, both of which have supported sonifications?

I don't know of any such namespaces that don't already have a native mapping. Am I missing something?

Wouldn't this call both sonification functions?

No -- it returns on the first successful conversion. I think this is a sane convention.

Wouldn't it be cleaner to just check if the annotation namespace is a key in SONIFY_MAPPING and if so call the function it maps to?

We'd need to explicitly code each namespace into a data structure, and I'd rather not do that.

For most namespace types, this isn't a huge deal, but for something like segment_* there are many namespaces (and potentially more in the future). Trying to do something programmatic to handle this case would eventually boil down to auto-conversion anyway.

@justinsalamon
justinsalamon Apr 25, 2016 Contributor

I don't know of any such namespaces that don't already have a native mapping. Am I missing something?

I don't have a specific example, just thinking of a hypothetical future case where you want to have different sonification functions for two annotations that can be converted into each other (imagine you decided to write a new/different function for pitch_midi and don't want pitch_hz annotations to be sonified using it and vise versa). Maybe this is a contrived case...

@bmcfee
bmcfee Apr 26, 2016 Contributor

a hypothetical future case where you want to have different sonification functions for two annotations that can be converted into each other

If two namespaces can be converted to each other, then they must be "equivalent" in a pretty strong sense. It's hard to imagine why we would have different sonification methods there.

However, I think I can imagine a slightly more realistic case in which conversion only works in one way. We currently have a 'click' sonifier that applies to all event-based data, for example, beats. We also have two beat namespaces, one with metrical position and one without. This conversion can go in only one direction: beat_position -> beat.

Now, imagine that we want to have a sonifier that plays different clicks on the downbeat. This would only apply to the beat_position namespace, but that can also be coerced into a beat namespace.

Proposed solution: use an ordered dict to store the mappings, and put the most specific namespaces first. The conversion logic stays the same, but sonification is always done with the most specific applicable method. If you want to sonify using an alternate method, you can always auto-convert the annotation before sonifying.

How does this sound?

@justinsalamon
justinsalamon Apr 26, 2016 Contributor

Proposed solution: use an ordered dict to store the mappings, and put the most specific namespaces first. The conversion logic stays the same, but sonification is always done with the most specific applicable method. If you want to sonify using an alternate method, you can always auto-convert the annotation before sonifying.

This could work, but the logic could get increasingly unwieldy as we add sonifications/namespaces/conversions. For example, in the future we might want to add destructive one-way conversions such as going from pitch_midi to onsets. The sonifications are dramatically different, and since onsets might come first in the ordered dictionary, it could get messy?

I actually don't think that a direct dictionary mapping from each namespace-to-sonification would be that bad... we don't have that many namespaces and it's not a quantity that's likely to grow very fast. And anyone adding a new namespace can make a PR that just adds the new namespace to the mapping dictionary, and since every namespace is mapped independently we don't have to worry about some new namespace or new conversion breaking the logic of the ordered dict. I actually think it's the simplest solution in the long run, but well, open to rebuttals as always.

@bmcfee
bmcfee Apr 26, 2016 Contributor

!resolved

@justinsalamon justinsalamon and 1 other commented on an outdated diff Apr 25, 2016
jams/tests/sonify_test.py
+ if duration is not None:
+ eq_(len(y), int(sr * duration))
+
+
+ for ns in ['segment_open', 'chord']:
+ for sr in [8000, 11025]:
+ for dur in [None, 5.0, 10.0]:
+ yield __test, ns, dur, sr
+
+
+def test_pitch_hz():
+ ann = jams.Annotation(namespace='pitch_hz')
+ ann.append(time=0, duration=1, value=261.0)
+ y = jams.sonify.sonify(ann, sr=8000, duration=2.0)
+
+ eq_(len(y), 8000 * 2)
@justinsalamon
justinsalamon Apr 25, 2016 Contributor

Missing tests for when the frequency value is 0 or negative

@bmcfee
bmcfee Apr 26, 2016 Contributor

!resolved

@justinsalamon
Contributor

@bmcfee I've added a bunch of comments. Don't hate me for asking, but wouldn't it make more sense for all the sonification code to live natively in jams rather than wrapping around mir_eval? The annotation viz module is a parallel case: does it make sense for it to live in mir_eval or jams?

On the one hand I'd argue that mir_eval is for evaluation whilst jams is for annotation generation/manipulation/exploration, and hence both sonification and visualization should be a native component of jams. On the other hand I can understand one would want to support sonification in mir_eval independently of jams, given that most people (still) don't use jams and would be left without sonification otherwise. I guess there are arguments in favor of both options...

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

Don't hate me for asking, but wouldn't it make more sense for all the sonification code to live natively in jams rather than wrapping around mir_eval?

If it wasn't already 90% done in mir_eval, it would make sense to do it in jams. But, since we already have mir_eval as a dependency, I'd prefer to not duplicate code.

The annotation viz module is a parallel case: does it make sense for it to live in mir_eval or jams?

I think for mir_eval, that would constitute bloat, since its main purpose is evaluation. (To channel Colin, sonification falls under "evaluation by inspection", hence its inclusion.) Viz could also fit in mir_eval, but since mir_eval has such limited scope in terms of what kinds of annotations it supports, I think it makes more sense to do it in jams. Or as a separate project altogether.

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

It seems like the last remaining issue on this is how to handle 0-duration intervals in pitch_hz sonification.

What do folks think about the following logic?

intervals, pitches = annotation.data.to_interval_values()

if np.all(intervals[:, 0] == intervals[:, 1]):
    intervals[:, :-1] = np.diff(intervals[:, 0])
    intervals[-1, -1] = annotation.duration

# proceed

That is, only impute durations if all observations have 0 duration. The last observation is inferred to span until the end of the annotation's valid timing interval (which need not be the entire track). Does this seem sane?

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

Implemented the above -- it works, but it sounds abysmal due to heavy modulation from the imputed intervals. I can't think of a good solution here, other than writing a special case synthesizer for continuous contours. The problem with that being that there's no good way to tell when to deploy that instead of the more general time_frequency synth.

@justinsalamon
Contributor
justinsalamon commented Apr 26, 2016 edited

Implemented the above -- it works, but it sounds abysmal due to heavy modulation from the imputed intervals. I can't think of a good solution here, other than writing a special case synthesizer for continuous contours. The problem with that being that there's no good way to tell when to deploy that instead of the more general time_frequency synth.

Just gave it a spin, unfortunately the generated audio is indeed unusable. It also took about 80 seconds to process a 35 second pitch_hz annotation, which is probably not reasonable. It looks like we do indeed require two different pitch sonification methods, one for "sparse" data and one for "dense" data. For dense data we could, for example, base it on melosynth.

The problem is, as you've noted, to determine which sonification function to use given an annotation. It's not a great solution, but perhaps we need to provide some optional parameter to allow the user to specify (something) to help disambiguate these cases?

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

The problem is, as you've noted, to determine which sonification function to use given an annotation. It's not a great solution, but perhaps we need to provide some optional parameter to allow the user to specify (something) to help disambiguate these cases?

I think this is getting at a deeper problem. We're trying to overload "pitch_hz" to do too many things: dense vs sparse, continuous (monophonic) vs overlapping/multi-f0, etc.

Maybe we just need to split the namespace, eg, pitch_hz_mono and pitch_hz_multi?

@justinsalamon
Contributor

Maybe we just need to split the namespace, eg, pitch_hz_mono and pitch_hz_multi?

I'm not sure that solves the problem: both mono and multi can be represented as sparse (note onset + duration) or dense (continuous f0) annotations. If anything it would have to be pitch_hz_sparse and pitch_hz_dense, or maybe pitch_hz_notes and pitch_hz_contour (the former always being sprase and the latter always dense).

An alternative option (for sonification) would be to change strategy and synthesize both sparse and dense pitch annotations by first sampling them using e.g. intervals_to_samples and then using a melosynth-like approach, instead of the gram approach.

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

If anything it would have to be pitch_hz_sparse and pitch_hz_dense, or maybe pitch_hz_notes and pitch_hz_contour (the former always being sprase and the latter always dense).

I like that last idea.

An alternative option (for sonification) would be to change strategy and synthesize both sparse and dense pitch annotations by first sampling them using e.g. intervals_to_samples and then using a melosynth-like approach, instead of the gram approach.

I don't understand what you mean. Melosynth wouldn't handle overlapping notes, right?

@justinsalamon
Contributor
justinsalamon commented Apr 26, 2016 edited

I like that last idea.

I'm happy to go with this - I don't think it makes sense to overload the same namespace with note and continuous f0 annotations, they're fundamentally different types of annotation, so splitting into 2 namespaces (well, 4, we'd have to split pitch_midi_note and pitch_midi_contour too) makes sense. Perhaps the names should be pitch_contour_[hz/midi] and pitch_note_[hz/midi] (i.e. first specify note/contour and then hz/midi), seems like that would make automatic conversion easier to implement?

I don't understand what you mean. Melosynth wouldn't handle overlapping notes, right?

No, but you could separate overlapping notes into individual tracks, synthesize each track separately and then mix together.

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

Perhaps the names should be pitch_contour_[hz/midi] and pitch_note_[hz/midi] (i.e. first specify note/contour and then hz/midi, seems like that would make automatic conversion easier to implement?

I don't think we need contour_midi -- if you've already quantized the frequencies to midi scale, grouping them into coherent intervals is easy/expected, right?

This leaves the question of what to do with the current pitch_hz namespace. Any suggestions?

At any rate, forking the pitch_hz namespace should be a separate PR. @justinsalamon care to start that thread?

No, but you could separate overlapping notes into individual tracks, synthesize each track separately and then mix together.

Sounds messy. :(

@justinsalamon
Contributor
justinsalamon commented Apr 26, 2016 edited

I don't think we need contour_midi -- if you've already quantized the frequencies to midi scale, grouping them into coherent intervals is easy/expected, right?

midi doesn't imply quantization, it's only the scale used to represent pitch (i.e. you can have continuous midi), the only difference is whether you represent pitch in hz or in midi note numbers, but otherwise the two are equivalent. I think it makes sense to first split on note/contour, and then split on hz/midi, so I'd advocate for 4 namespaces. It also simplifies conversion: pitch_note_[hz/midi] is bi-directional and pitch_contour_[hz/midi] is bi-directional, and you can convert pitch_note_* to pitch_contour_* but not in the other direction.

This leaves the question of what to do with the current pitch_hz namespace. Any suggestions?

What do you mean? If we're splitting it up into a several namespaces, shouldn't we just eliminate it?

At any rate, forking the pitch_hz namespace should be a separate PR. @justinsalamon care to start that thread?

Happy to, but I think i'm running out of cycles. Once I have the cluster spinning and am waiting for results I could look into it.

Sounds messy. :(

Yeah, it could be.

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

I think it makes sense to first split on note/contour, and then split on hz/midi, so I'd advocate for 4 namespaces. It also simplifies conversion: pitch_note_[hz/midi] is bi-directional and pitch_contour_[hz/midi] is bi-directional, and you can convert pitch_note_* to pitch_contour_* but not in the other direction.

In that case, why not drop "pitch" from the name and just call them "contour__" or "notes__"? Alternately, why not just keep "pitch" for dense data and use "notes" for sparse data?

What do you mean? If we're splitting it up into a several namespaces, shouldn't we just eliminate it?

That would destroy backwards compatibility.

Happy to, but I think i'm running out of cycles. Once I have the cluster spinning and am waiting for results I could look into it.

I'm not asking for code, just start the issue and write down your thoughts -- seeing as you're the expert here, it would make more sense than my spin on things.

@justinsalamon
Contributor

In that case, why not drop "pitch" from the name and just call them "contour_" or "notes_"? Alternately, why not just keep "pitch" for dense data and use "notes" for sparse data?

Yes, we could just have notes_hz/midi and contour_hz/midi, though "contour" is a tad more ambiguous than "pitch_contour". I have no strong preference.

That would destroy backwards compatibility.

True, but someone could always checkout an older version (i.e. the current version) if they wanted support for this namespace. I'm not sure it makes sense to support backwards compatibility at this early stage where things are still morphing quite often, especially of our initial namespace was broken?

I'm not asking for code, just start the issue and write down your thoughts -- seeing as you're the expert here, it would make more sense than my spin on things.

Ah sure, on it.

@bmcfee
Contributor
bmcfee commented Apr 26, 2016

Yes, we could just have notes_hz/midi and contour_hz/midi, though "contour" is a tad more ambiguous than "pitch_contour". I have no strong preference.

I'd vote for pitch_* and notes_* then. I think that's pretty straightforward without being terribly confusing.

True, but someone could always checkout an older version (i.e. the current version) if they wanted support for this namespace. I'm not sure it makes sense to support backwards compatibility at this early stage where things are still morphing quite often, especially of our initial namespace was broken?

Forcing people to use old versions is always a bad policy, if only for keeping folks up to date with bugfixes.

I'm fine with deprecating things, but we need to give folks a runway to transition their data within a functioning environment. My usual policy is to deprecate features within minor revisions -- with warnings to that effect -- and remove them across major revisions.

@justinsalamon
Contributor

I'd vote for pitch_* and notes_* then. I think that's pretty straightforward without being terribly confusing.

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 note_* and contour_* or pitch_note_* and pitch_contour_*.

I'm fine with deprecating things, but we need to give folks a runway to transition their data within a functioning environment. My usual policy is to deprecate features within minor revisions -- with warnings to that effect -- and remove them across major revisions.

That sounds fine to me, as long as we eventually kill pitch_hz altogether :)

@bmcfee
Contributor
bmcfee commented May 25, 2016

Circling back on this, since #111 suggests that we're going to deprecate pitch_* in favor of more specific namespaces.

I think the piano roll sonification that we currently have will work fine for note_* annotations, but not contour_* annotations. As @justinsalamon mentioned previously, we could roll our own sonifier for continuous pitch annotations (and/or steal from melosynth). The question I have is: does this constitute bloat? I suspect it might make better sense to PR melosynth into mir_eval, assuming that @craffel would find it useful.

Implementing it within jams seems a little to special-case to me, and borders on bloat. What do y'all think @justinsalamon ?

@justinsalamon
Contributor
justinsalamon commented May 25, 2016 edited

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.

@craffel
craffel commented May 25, 2016

Of course one could argue that evaluation by audition/visualization is relevant to mir_eval.

That's why it was added.

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.

This is important. Whatever the visualization/sonification tool is, it should (IMO) be storage format-independent.

For historical reasons right now visualization lives inside JAMS and sonification lives inside mir_eval, though I think that's pretty arbitrary.

Visualization was always intended to be part of mir_eval, but no one has gotten around to adding it.

@justinsalamon
Contributor

Visualization was always intended to be part of mir_eval, but no one has gotten around to adding it.

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.

@craffel
craffel commented May 25, 2016

It's been part of the plan for 2 years, at least: craffel/mir_eval#28

@justinsalamon
Contributor

It's been part of the plan for 2 years, at least: craffel/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.

@bmcfee
Contributor
bmcfee commented May 25, 2016

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.

@craffel
craffel commented May 25, 2016

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.

Well, like I said, as long as the visualizers are not tied to a specific data storage format, it's fine either way.

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).

This seems to make sense to me.

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.

Sure, that would be great to have. Realistically I won't do it anytime in the foreseeable future.

@bmcfee
Contributor
bmcfee commented May 25, 2016

Realistically I won't do it anytime in the foreseeable future.

understood; I'll try to find time for it soonish.

@bmcfee bmcfee referenced this pull request in craffel/mir_eval May 25, 2016
Closed

continuous pitch sonification #194

@bmcfee
Contributor
bmcfee commented Jun 8, 2016

Note to self: this PR is stalled by the following:

  • pitch namespace revisions
  • mir_eval 0.4 release
@bmcfee bmcfee changed the title from Sonify to [WIP] Sonify Jun 8, 2016
@bmcfee
Contributor
bmcfee commented Jul 19, 2016

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.

@bmcfee
Contributor
bmcfee commented Aug 29, 2016

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, note_hz).

Practically, this means that only annotations of type pitch_contour will be sonified using contours. I think this is the expected behavior, but I wanted to make it 100% clear in the comments here.

@bmcfee bmcfee changed the title from [WIP] Sonify to Sonify Aug 29, 2016
@bmcfee
Contributor
bmcfee commented Aug 30, 2016

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


Comments from Reviewable

@bmcfee
Contributor
bmcfee commented Aug 30, 2016

This is basically done, modulo two things:

  • beat_position -> beats+downbeat sonification with different click frequencies
  • multi_segment -> similarly

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?

@bmcfee
Contributor
bmcfee commented Aug 30, 2016

downbeats (beat_position) and multi_segment are now in.

I think this covers everything except for the ones that don't make sense to sonify: key, tag_*, tempo, etc.

Anyone care for a final CR?

bmcfee added some commits Dec 12, 2015
@bmcfee bmcfee first cut at sonification via mir_eval
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
632b514
@bmcfee bmcfee updated sonify to use namespace conversion
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
4cf9dc4
@bmcfee bmcfee updated contours, added piano roll sonification
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
46c2042
@bmcfee bmcfee added multisegment sonification
f7d9c04
@bmcfee
Contributor
bmcfee commented Sep 2, 2016

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


Comments from Reviewable

@bmcfee bmcfee merged commit 0868430 into master Sep 2, 2016

5 checks passed

code-review/reviewable 5 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 99.728%
Details
dependency-ci Dependencies checked
Details
@bmcfee bmcfee deleted the sonify branch Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment