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 for mid date interpolation #2757

Merged
merged 6 commits into from Jun 20, 2018

Conversation

Projects
None yet
4 participants
@douglasmacdonald
Copy link
Contributor

douglasmacdonald commented Jun 1, 2018

steps[0, 1:-1:2] = steps[0, 2::2] = (x[:-1] + x[1:]) / 2

This does not work for interpolating datetimes. This is because adding date does not makes sense and Numpy will throw an error

TypeError: ufunc add cannot use operands with types dtype('<M8[us]') and dtype('<M8[us]')

By changing it to an equivalent form

steps[0, 1:-1:2] = steps[0, 2::2] = x[:-1] + (x[1:] - x[:-1])/2

it adds a datetime and a delta datetime.

This works equally well for numbers.

With a bit of help I could also write tests.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jun 1, 2018

Very clever! Thanks for the fix! The failing test doesn't seem related?

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 1, 2018

I've been puzzling over this for a while so this is great. Do the pre- and post-step function need similar fixes or do they already work with datetimes?

@douglasmacdonald

This comment has been minimized.

Copy link
Contributor Author

douglasmacdonald commented Jun 1, 2018

For the other interpolations datetimes should just work. I think I have tested this in the past but have just had a look and I think the others just use indexing and do not need to calculate a mid-point.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 1, 2018

Great thanks. It would be great to add some tests for this in tests/operation/testoperation.py. They'd look something like:

    def test_interpolate_datetime_curve_mid(self):
        datetimes = ...
        values = ...
        interpolated = interpolate_curve(Curve((datetimes, values)), interpolation='steps-mid')
        curve = Curve(...)
        self.assertEqual(interpolated, curve)
@douglasmacdonald

This comment has been minimized.

Copy link
Contributor Author

douglasmacdonald commented Jun 2, 2018

Okay. Will write some tests.

@philippjfr philippjfr force-pushed the douglasmacdonald:interpolating_mid_datetime branch from 7e9d417 to ef9bcf7 Jun 15, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 15, 2018

I've made some other changes by casting the data to ints/floats before applying the interpolation and then casting back. This has the benefit that it also works for pandas period types and others, but may mess with types that cannot be represented with a datetime64[ns] type. In any case, it's a major improvement and I've also added tests.

@jbednar

This comment has been minimized.

Copy link
Contributor

jbednar commented Jun 15, 2018

If we find types that don't work, maybe we can add some exception-handling fallback to try another approach in those cases.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 15, 2018

Made another change, it'll now use the native resolution if possible and only fall back to nanosecond resolution if the data is not a datetime64 type.

@douglasmacdonald

This comment has been minimized.

Copy link
Contributor Author

douglasmacdonald commented Jun 15, 2018

I promise I was going to add tests. However, you are wise in not waiting, it would take me a while before I got the time. Thank you.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 15, 2018

I promise I was going to add tests. However, you are wise in not waiting, it would take me a while before I got the time.

Haha, no problem, I know how it is and I thought it would be nice to get this into 1.10.6.

@philippjfr philippjfr added this to the v1.10.6 milestone Jun 15, 2018

@douglasmacdonald

This comment has been minimized.

Copy link
Contributor Author

douglasmacdonald commented Jun 15, 2018

This is an observation not a suggestion to change anything. If you are now casting to number before the interpolation then the fix I gave might not be necessary. As I did not know the greater context, I think I avoided casting in an attempt to touch as little code as possible and easier to fit in. However, your method will be a better fit with the project.

Thank you for getting the changes in quickly.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 20, 2018

Thanks, I think we're fine leaving it the way it is now. Ready to review/merge.

douglasmacdonald and others added some commits Jun 1, 2018

Fix for mid date interpolation
`steps[0, 1👎2] = steps[0, 2::2] = (x[:-1] + x[1:]) / 2`

This does not work for interpolating datetimes. This is because adding date does not makes sence and Numpy will through an error

`TypeError: ufunc add cannot use operands with types dtype('<M8[us]') and dtype('<M8[us]')`

By changing it to an equivalent form

steps[0, 1👎2] = steps[0, 2::2] = x[:-1] + (x[1:] - x[:-1])/2

it adds a datetime and a delta datetime.

This works equally well for numbers.

@philippjfr philippjfr force-pushed the douglasmacdonald:interpolating_mid_datetime branch from 6da9c4e to 9866597 Jun 20, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 20, 2018

Now also correctly handled additional value dimensions.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 20, 2018

PR build failure can be ignored, it's not pulling the latest data for some reason.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jun 20, 2018

PR build failure can be ignored, it's not pulling the latest data for some reason.

Agreed and the PR looks good. Thanks @douglasmacdonald!

@jlstevens jlstevens merged commit 502e9d3 into pyviz:master Jun 20, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on interpolating_mid_datetime at 83.7%
Details
s3-reference-data-cache Test data is cached.
Details

@douglasmacdonald douglasmacdonald deleted the douglasmacdonald:interpolating_mid_datetime branch Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.