search

marl / jams

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

Fixed annotation.slice timing and docstring examples #189

Closed bmcfee closed 6 years ago

bmcfee commented 6 years ago

I fixed this just by changing the ann_slice.data line to ann_slice.to_dataframe(). Fixes #174

This shouldn't need a CR, but I did notice that the output in the examples was incorrect. When it prints ann_slice.time, ann_slice.duration, it was giving (0, 3) in the docstring. When I run the code, it gives (5, 3). @justinsalamon which of these should be considered correct?

justinsalamon commented 6 years ago

The correct behavior should be (0, 3) as per the original docstring: slicing not only trims the annotation, it also updates the time of all events with respect to the new start time, which is now considered the new zero. So if you have an annotation with time,duration=2,8, and you slice(5,8), then the new annotation should have time,duration=0,3.

Could it be that slice() was broken when jams was overhauled to remove the pandas dependency and it didn't get caught by the unit tests?

bmcfee commented 6 years ago

Could it be that slice() was broken when jams was overhauled to remove the pandas dependency and it didn't get caught by the unit tests?

It sure looks that way. I'll roll a fix and expanded test into this PR.

bmcfee commented 6 years ago

@justinsalamon slice is fixed and behaving properly now. I think that should do it here.

justinsalamon commented 6 years ago

It looks like there's also some erroneous behavior in trim? For example:

In [1]: import jams

In [2]: ann = jams.Annotation(namespace='tag_open', time=5, duration=10)

In [3]: ann.trim(8, 20) Out[3]: <Annotation(namespace='tag_open', time=8, duration=7, annotation_metadata=<AnnotationMetadata(...)>, data=<0 observations>, sandbox=<Sandbox(...)>)> I get where time=8 comes from, but duration=7 seems wrong. I would expect either 2 (since the slice end is off the end of the input annotation) or 12 (end_time - start_time), but can't figure out where 7 comes from.

7 is the correct value = the original annotation covered the time range [5,15]. If you call trim(8,20), the new start time is 8 but the end time doesn't change because it's already smaller than the specified trim region, i.e. the new end time remains 15s, which means duration=7 with respect to the new start time of the annotation (8).

bmcfee commented 6 years ago

Yeah, I'd deleted that comment because it was a thinko. Everything seems square now, but a once-over the changes would be appreciated.