search

marl / jams

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

Boundary condition on trimming #204

Closed bmcfee closed 5 years ago

bmcfee commented 5 years ago

This PR fixes #203 by including observations which have end time coincident with the trimming start time.

I'm still not 100% sure this is completely correct. It works for @tomxi's example case, where there was a time=0, duration=0 observation being trimmed to start at 0. This should clearly be included.

However, if we have an observation [2, 2+1) with a trim of [3, 10), the modified logic will allow it to pass through, when it probably shouldn't, since [2, 3) ∩ [3, 10) = {}. The ambiguity here comes from our special-case handling of duration-0 observations, which we treat as closed intervals to allow a (Nyquist-style) sampling interpretation.

Perhaps we should explicitly handle this case, by only allowing the boundary condition end_time >= trim_start if the observation duration is 0?

tomxi commented 5 years ago

Perhaps we should explicitly handle this case, by only allowing the boundary condition end_time >= trim_start if the observation duration is 0?

I would second this option.

bmcfee commented 5 years ago

@justinsalamon what do you think?

justinsalamon commented 5 years ago

Sounds like a reasonable solution to me (explicitly handling), given that for events with any non-zero duration this becomes a non-issue.

bmcfee commented 5 years ago

Ok, done. Docs updated as well.

bmcfee commented 5 years ago

Shall we merge this then?

tomxi commented 5 years ago

Not sure if it counts for anything.... but I took a quick peek at the changed files and it all LGTM. Thanks for addressing this.

bmcfee commented 5 years ago

Looks like I dropped the ball on this, but at ~3 months it looks fine to me. :shipit: