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.

Sign up for GitHub

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

Per series roll period #811

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Per series roll period #811

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
adaa6ca
Per series roll period
evamedia Dec 24, 2016
41047fb
Added auto_test
evamedia Dec 30, 2016
8c386f1
Adding test html example
evamedia Dec 30, 2016
717ff0b
Style guide updates
evamedia Dec 30, 2016
c57720f
Changes Requested
evamedia Dec 30, 2016
ab6575d
removed a few tabs
evamedia Dec 31, 2016
28c3e15
var not const
evamedia Jan 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions auto_tests/tests/per_series_rolling_average.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/**
* @fileoverview Tests for per series rolling averages.
*
* @author Nick Hayday ([email protected])
*/

import Dygraph from '../../src/dygraph';

import Util from './Util';

describe("per-series-rolling-average", function() {

cleanupAfterEach();

it('testPerSeriesRollingAverage', function() {
var opts = {
width: 480,
height: 320,
series: {
Y: {
rollPeriod: 1 }
}
};
var data = "X,Y,Z\n" +
"0,0,0\n" +
"1,1,10\n" +
"2,2,20\n" +
"3,3,30\n"
;

var graph = document.getElementById("graph");
var g = new Dygraph(graph, data, opts);

g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 1 Z: 10", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 2 Z: 20", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 3 Z: 30", Util.getLegend());
assert.equal(1, g.getOption("rollPeriod", "Y"));

var seriesOpt = {};
seriesOpt["Y"] = { rollPeriod: 2 };
g.updateOptions ({ series: seriesOpt });
Copy link
Owner

Choose a reason for hiding this comment

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

Make the updateOptions calls self-contained by writing them as:

g.updateOptions({
  series: { Y: { rollPeriod: 2 } }
});

and killing the seriesOpt variable (here and below).


g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 0.5 Z: 10", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 1.5 Z: 20", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 2.5 Z: 30", Util.getLegend());
assert.equal(2, g.getOption("rollPeriod", "Y"));

seriesOpt["Y"] = { rollPeriod: 3 };
seriesOpt["Z"] = { rollPeriod: 2 };
g.updateOptions ({ series: seriesOpt });
g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 0.5 Z: 5", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 1 Z: 15", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 2 Z: 25", Util.getLegend());
assert.equal(3, g.getOption("rollPeriod", "Y"));
assert.equal(2, g.getOption("rollPeriod", "Z"));

seriesOpt["Y"] = { rollPeriod: 1 };
seriesOpt["Z"] = { rollPeriod: 4 };
g.updateOptions ({ series: seriesOpt });
g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 1 Z: 5", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 2 Z: 10", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 3 Z: 15", Util.getLegend());
assert.equal(1, g.getOption("rollPeriod", "Y"));
assert.equal(4, g.getOption("rollPeriod", "Z"));

Copy link
Owner

Choose a reason for hiding this comment

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

style nit: remove these blank lines





});

it('testDefaultToGlobalRollingAverage', function() {
var opts = {
width: 480,
height: 320,
rollPeriod: 2,
series: {
Y: {
Copy link
Owner

Choose a reason for hiding this comment

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

style nit: fix indentation here

rollPeriod: 1 }
}
};
var data = "X,Y,Z\n" +
"0,0,0\n" +
"1,1,10\n" +
"2,2,20\n" +
"3,3,30\n"
;

var graph = document.getElementById("graph");
var g = new Dygraph(graph, data, opts);

g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 1 Z: 5", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 2 Z: 15", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 3 Z: 25", Util.getLegend());
assert.equal(1, g.getOption("rollPeriod", "Y"));
assert.equal(2, g.getOption("rollPeriod", "Z"));
assert.equal(2, g.rollPeriod());
Copy link
Owner

Choose a reason for hiding this comment

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

We should get rid of the rollPeriod() method. getOption('rollPeriod') does the same thing but also lets you get per-series values.


var seriesOpt = {};
seriesOpt["Y"] = { rollPeriod: 3 };
g.updateOptions ({ series: seriesOpt });

g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 0.5 Z: 5", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 1 Z: 15", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 2 Z: 25", Util.getLegend());
assert.equal(3, g.getOption("rollPeriod", "Y"));
assert.equal(2, g.getOption("rollPeriod", "Z"));
assert.equal(2, g.rollPeriod());

g.updateOptions ({ rollPeriod: 3 });
g.setSelection(0); assert.equal("0: Y: 0 Z: 0", Util.getLegend());
g.setSelection(1); assert.equal("1: Y: 0.5 Z: 5", Util.getLegend());
g.setSelection(2); assert.equal("2: Y: 1 Z: 10", Util.getLegend());
g.setSelection(3); assert.equal("3: Y: 2 Z: 20", Util.getLegend());
assert.equal(3, g.getOption("rollPeriod", "Y"));
assert.equal(3, g.getOption("rollPeriod", "Z"));
Copy link
Owner

Choose a reason for hiding this comment

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

This should be 2 to be consistent with how other per-series options work. See http://jsfiddle.net/eM2Mg/9158/

Copy link
Author

Choose a reason for hiding this comment

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

Not sure I understand "Y" is 3 because it was set previously, "Z" is 3 because the global rollPeriod was updated and "Z" hadn't been set separately. On the example initial strokeWidth : 4 for "Z" then the global StrokeWidth is changed to 1 , leaving "X" at 2

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, nevermind. I misread this.

Could you add a test that makes sure updating the global rollPeriod doesn't affect a per-series rollPeriod?

assert.equal(3, g.rollPeriod());
});


});
13 changes: 11 additions & 2 deletions src/dygraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2026,14 +2026,23 @@ Dygraph.prototype.predraw_ = function() {

this.cascadeEvents_('predraw');

var seriesName = this.getLabels();

// Convert the raw data (a 2D array) into the internal format and compute
// rolling averages.
this.rolledSeries_ = [null]; // x-axis is the first series and it's special
for (var i = 1; i < this.numColumns(); i++) {
// var logScale = this.attr_('logscale', i); // TODO(klausw): this looks wrong // konigsberg thinks so too.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you get rid of the rollPeriod_ member variable altogether?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean the whole library?

var series = this.dataHandler_.extractSeries(this.rawData_, i, this.attributes_);
if (this.rollPeriod_ > 1) {
series = this.dataHandler_.rollingAverage(series, this.rollPeriod_, this.attributes_);
var seriesRollPeriod;

Copy link
Owner

Choose a reason for hiding this comment

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

style nit: remove blank line & fix indentation

if (this.getOption("rollPeriod", seriesName[i]) == undefined) {
seriesRollPeriod = this.rollPeriod();
} else {
seriesRollPeriod = this.getOption("rollPeriod", seriesName[i]);
}
if (seriesRollPeriod > 1) {
series = this.dataHandler_.rollingAverage(series, seriesRollPeriod, this.attributes_);
}

this.rolledSeries_.push(series);
Expand Down
71 changes: 71 additions & 0 deletions tests/per-series-rolling-average.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<!DOCTYPE html>
<html>
<head>
<link rel="stylesheet" href="../css/dygraph.css">
<title>Per Series Rolling Average</title>
<script type='text/javascript' src='https://code.jquery.com/jquery-1.11.3.min.js'></script>
<script type="text/javascript" src="../dist/dygraph.js"></script>
<script type="text/javascript" src="data.js"></script>
<style type="text/css">
#graphdiv {
width:600px;
height: 300px;
left: 10px;
right: 10px;
top: 40px;
bottom: 10px;
}
</style>
</head>
<body>

Copy link
Owner

Choose a reason for hiding this comment

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

style nit: use a two space indent in this file

<div id="graphdiv"> </div>

Check the boxes to apply the rolling average <br>

<input type="checkbox" href="#" id="high" name="high" />High (7 day rolling average)<br>
<input type="checkbox" href="#" id="low" name="low" />Low (30 day rolling average)

<script type="text/javascript">

var g = new Dygraph(document.getElementById('graphdiv'), data,
{
title: 'Per Series Rolling Average',
xlabel: 'Date',
ylabel: 'Temperature (F)'

});

function updateRollPeriod(series, value) {

var seriesOpt = {};
seriesOpt[series] = { rollPeriod: value };
g.updateOptions ({ series: seriesOpt });

}



$("#high").change(function () {
if(this.checked) {
updateRollPeriod("High", 7);
} else {
updateRollPeriod("High", 1);
}
});

$("#low").change(function () {
if(this.checked) {
updateRollPeriod("Low", 30);
} else {
updateRollPeriod("Low", 1);
}
});


</script>
</body>
</html>