Adds difference function

[mboelstra]

• Adds a new aggregation function called range
• Bugfix in DerivativeAggregator

[pauldix]

Can you add tests for both the bug fix and the range function?

[mboelstra]

Hi Paul,

No problem! But could you help me and point me the proper file(s) or test suite(s) for adding these tests? It seems like I can’t find the other/existing aggregation tests…

-Matthijs

On 16 mei 2014, at 16:07, Paul Dix notifications@github.com wrote:

Can you add tests for both the bug fix and the range function?

—
Reply to this email directly or view it on GitHub.

[pauldix]

Sure, look at src/integration/data_test.go. That's where these tests should probably live. To run that do:

make integration_test only=DataTest verbose=on

Or run the whole suite with

make test

Thanks @mboelstra!

[mboelstra]

@pauldix I've added the tests you requested. Please review
Thanx

[jvshahid]

@mboelstra do you mind signing the CLA so I can merge your pr into master.

[mboelstra]

@jvshahid The CLA is signed

[jvshahid]

Just a couple of things before I merge the pr:

1. The third test has a TODO but it seems like the test suite is passing. Is it still needed ?
2. I feel like range should be the difference between the minimum value and maximum value instead of first and last. What do you think ?

[mboelstra]

Good questions:

1. This test is commented out because it would fail if it was enabled. Don’t know what the policy is for these kind of TODO’s.
2. In our use case we’re dealing with time series data that is always increasing (a running total of a kWh-meter) and in such cases the outcome would be the same. However when the time series data is not strictly increasing it matters.

Consider the example where a kWh-meter can count in both directions and within a time bucket the total goes up first, then down and then up again (e.g.: 5,6,7,6,5,4,3,2,1,2,3,4,5,6,7,8,9,10).
In this example the nett delta is 5 (10 - 5 = 5). If we would have used the min and max the result would be 9 (10 - 1 = 9) which is incorrect.
Also the sum of all delta’s should equal the delta of the first and last point in the series. In other words no matter what the time interval is for the group by function, the sum of all results should be equal to the delta of the first and last point in the series.

The current implementation is not perfect since it fails in cases where the time interval of the group by function equals the time between each point and the points line-up with the bucket times.
I’m planning to fix this as well but I need a little more time for this.

I hope my explanation is clear enough (since I’m not a native English speaker).

On 20 mei 2014, at 18:49, John Shahid notifications@github.com wrote:

Just a couple of things before I merge the pr:

The third test has a TODO but it seems like the test suite is passing. Is it still needed ?
I feel like range should be the difference between the minimum value and maximum value instead of first and last. What do you think ?
—
Reply to this email directly or view it on GitHub.

[jvshahid]

Oh i c, i missed the block comment. Can you please change that to line comments and squash your commits in one commit.

[jvshahid]

Ok, let's call it difference if it's just a difference between the first and last points. I'll merge the pr as soon as these changes are made. Thanks for your contribution.

[mboelstra]

Much better to call it difference. Looked at the R documentation and they also call this a difference function ;) . Also squashed everything to one commit.

[jvshahid]

Awesome, i'll wait for travis ci to finish testing and will merge it if everything looks fine. Thanks again.