Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #248, support gaps in labeled interval interpolation #249

Merged
merged 4 commits into from
Apr 17, 2017

Conversation

bmcfee
Copy link
Collaborator

@bmcfee bmcfee commented Apr 14, 2017

This PR rewrites util.interpolate_intervals to support gaps in the intervals properly.

For now, I added documentation to indicate that time_points should be in ascending order. We can either tighten this by adding a validation check, or loosen it and extend the function to support unordered time grids.

@bmcfee bmcfee added this to the 0.5 milestone Apr 14, 2017
@bmcfee bmcfee self-assigned this Apr 14, 2017
@bmcfee bmcfee requested a review from craffel April 14, 2017 19:32
@bmcfee
Copy link
Collaborator Author

bmcfee commented Apr 14, 2017

Note: coverage percentage decrease here is because the rewrite of interpolate_intervals is shorter than the previous implementation.

Copy link
Collaborator

@craffel craffel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, elegant solution. Can you add a test which checks that it has the expected behavior with non-contiguous intervals?

@bmcfee bmcfee force-pushed the ival-to-sample-gaps branch from 9f0cba1 to e695ece Compare April 17, 2017 13:10
@bmcfee
Copy link
Collaborator Author

bmcfee commented Apr 17, 2017

Nice, elegant solution. Can you add a test which checks that it has the expected behavior with non-contiguous intervals?

Done.

@craffel craffel merged commit b372606 into mir-evaluation:master Apr 17, 2017
@craffel
Copy link
Collaborator

craffel commented Apr 17, 2017

Thanks!

craffel pushed a commit that referenced this pull request Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants