Add linear regression functions #1063

benraskin92 · 2018-10-10T14:55:22Z

No description provided.

nikunjgit · 2018-10-10T15:28:15Z

src/query/functions/temporal/linear_regression.go

+		}
+
+	case DerivType:
+		if len(args) != 1 {


you forgot to assign duration here. Bug ?

All good- duration is not needed for deriv

codecov · 2018-10-10T17:53:48Z

Codecov Report

Merging #1063 into master will decrease coverage by 0.01%.
The diff coverage is 88.4%.

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
- Coverage    77.4%   77.39%   -0.02%     
==========================================
  Files         545      546       +1     
  Lines       46068    46132      +64     
==========================================
+ Hits        35659    35702      +43     
- Misses       8156     8168      +12     
- Partials     2253     2262       +9

Flag	Coverage Δ
#aggregator	`81.59% <ø> (ø)`	⬆️
#collector	`59.23% <ø> (ø)`	⬆️
#dbnode	`81.29% <ø> (-0.05%)`	⬇️
#m3em	`73.21% <ø> (ø)`	⬆️
#m3ninx	`75.25% <ø> (ø)`	⬆️
#m3nsch	`51.19% <ø> (ø)`	⬆️
#query	`63.67% <88.4%> (+0.18%)`	⬆️
#x	`75.1% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a10b8e...281f999. Read the comment docs.

arnikola · 2018-10-12T09:49:43Z

src/query/functions/temporal/base.go

@@ -299,6 +299,7 @@ func (c *baseNode) processSingleRequest(request processRequest) error {
 			values = append(values, val)
 			newVal := math.NaN()
 			alignedTime, _ := bounds.TimeForIndex(i)
+			// fmt.Println("align: ", alignedTime.String())


nit; delete

arnikola · 2018-10-12T10:03:28Z

src/query/functions/temporal/linear_regression.go

+			return emptyOp, fmt.Errorf("invalid number of args for %s: %d", PredictLinearType, len(args))
+		}
+
+		duration, ok = args[1].(float64)


What's arg[0]?

The duration for the actual query.

arnikola · 2018-10-12T10:10:27Z

src/query/functions/temporal/linear_regression.go

+
+	var slope, intercept float64
+
+	if l.op.operatorType == PredictLinearType {


This reads a little weird to me, since you're splitting processing amongst two functions and doing related calculations in both.

Instead, can you pull the code from linearRegression() into this Process function, and let it do the switching on its own?

Alternatively, if you want to avoid dealing with operator type inside the regression function, you can pass a bool that signifies wether you're calculating regression or deriv into there, and have it just return a float, something like linearRegression(isDeriv bool, dps ts.Datapoints, interceptTime time.Time) float64

I like separate linearRegression since it makes it easy to test. My above comment about having a separate func instead duration should fix this problem.

arnikola · 2018-10-12T10:13:23Z

src/query/functions/temporal/linear_regression.go

+func linearRegression(dps ts.Datapoints, interceptTime time.Time) (float64, float64) {
+	var (
+		n            float64
+		sumX, sumY   float64


What are X and Y?

arnikola · 2018-10-12T10:13:37Z

src/query/functions/temporal/linear_regression.go

+		sumX, sumY   float64
+		sumXY, sumX2 float64
+
+		nonNaNCount int


nit: maybe valueCount?

arnikola · 2018-10-12T10:16:02Z

src/query/functions/temporal/linear_regression.go

+		nonNaNCount++
+
+		// convert to milliseconds
+		x := float64(dp.Timestamp.Sub(interceptTime).Nanoseconds()/1000000) / 1e3


I think this converts it to seconds, not millis. Can you just do float64(dp.Timestamp.Sub(interceptTime).Seconds())?

nikunjgit · 2018-10-12T19:37:01Z

src/query/functions/temporal/base.go

@@ -360,7 +361,7 @@ func (c *baseNode) sweep(processedKeys []bool, maxBlocks int) {

 // Processor is implemented by the underlying transforms
 type Processor interface {
-	Process(values ts.Datapoints) float64
+	Process(values ts.Datapoints, alignedTime time.Time) float64


call it evaluationTime ?

nikunjgit · 2018-10-12T19:43:18Z

src/query/functions/temporal/linear_regression.go

+)
+
+type linearRegressionProcessor struct {
+	duration float64


instead of this, store a function here which takes in a slope and intercept, returns a float. This way you don't have to compare Op in Process.

nikunjgit · 2018-10-12T19:43:50Z

src/query/functions/temporal/linear_regression.go

+
+	var slope, intercept float64
+
+	if l.op.operatorType == PredictLinearType {


I like separate linearRegression since it makes it easy to test. My above comment about having a separate func instead duration should fix this problem.

nikunjgit · 2018-10-12T21:11:15Z

src/query/functions/temporal/linear_regression.go

+	var (
+		n            float64
+		sumX, sumY   float64
+		sumXY, sumX2 float64


better name ?

nikunjgit · 2018-10-12T21:11:24Z

src/query/functions/temporal/linear_regression.go

+		n            float64
+		sumX, sumY   float64
+		sumXY, sumX2 float64
+


remove line ?

nikunjgit · 2018-10-12T21:11:29Z

src/query/functions/temporal/linear_regression.go

+		}
+
+		valueCount++
+


remove line ?

nikunjgit · 2018-10-12T21:11:40Z

src/query/functions/temporal/linear_regression.go

+	}
+
+	slope, intercept := linearRegression(dps, evaluationTime, l.isDeriv)
+


remove line ?

benraskin92 requested review from nikunjgit and arnikola October 10, 2018 14:55

nikunjgit reviewed Oct 10, 2018

View reviewed changes

arnikola reviewed Oct 12, 2018

View reviewed changes

nikunjgit reviewed Oct 12, 2018

View reviewed changes

benraskin92 force-pushed the braskin/linear_regression branch from fcdd004 to 57016fb Compare October 12, 2018 20:53

nikunjgit approved these changes Oct 12, 2018

View reviewed changes

Benjamin Raskin added 11 commits October 13, 2018 12:13

Add linear regression functions

7f37525

More changes

92a83ec

Add tests

81778b2

Fix linear_predict logic

cd46282

Address comments

b4b42a3

Name change

e63db42

Address comments

74b559e

More name changes

67cf587

debug stuff

546246f

TABS!

4086614

Undo debug

281f999

benraskin92 force-pushed the braskin/linear_regression branch from 13e976a to 281f999 Compare October 13, 2018 16:13

benraskin92 merged commit 03b0242 into master Oct 13, 2018

benraskin92 deleted the braskin/linear_regression branch October 13, 2018 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add linear regression functions #1063

Add linear regression functions #1063

benraskin92 commented Oct 10, 2018

nikunjgit Oct 10, 2018

benraskin92 Oct 10, 2018

codecov bot commented Oct 10, 2018 •

edited

Loading

arnikola Oct 12, 2018

arnikola Oct 12, 2018

benraskin92 Oct 12, 2018

arnikola Oct 12, 2018

nikunjgit Oct 12, 2018

arnikola Oct 12, 2018

arnikola Oct 12, 2018

arnikola Oct 12, 2018

nikunjgit Oct 12, 2018

nikunjgit Oct 12, 2018

nikunjgit Oct 12, 2018

nikunjgit Oct 12, 2018

nikunjgit Oct 12, 2018

nikunjgit Oct 12, 2018

nikunjgit Oct 12, 2018


		var slope, intercept float64

		if l.op.operatorType == PredictLinearType {

		}

		slope, intercept := linearRegression(dps, evaluationTime, l.isDeriv)

Add linear regression functions #1063

Add linear regression functions #1063

Conversation

benraskin92 commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2018 •

edited

Loading