Added support for stepPlot per-series #204

eichsjul · 2013-01-31T14:18:16Z

Hello,

we (Sauter) have decided to work with dygraphs for one of our products because it really seems to be a good soltution!
We saw your to do and added the feature enabling series based definition of the step plot functionality.

Best regards

kberg · 2013-01-31T14:29:19Z

dygraph-canvas.js

@@ -261,7 +261,12 @@ DygraphCanvasRenderer._drawStyledLine = function(e,
    drawPointCallback, pointSize) {
  var g = e.dygraph;
  // TODO(konigsberg): Compute attributes outside this method call.
-  var stepPlot = g.getOption("stepPlot");  // TODO(danvk): per-series
+  var stepPlot = g.getOption("stepPlot");


Try g.attributes_.getForSeries("stepPlot", e.setName);

We don't want private things to leak outside Dygraphs, and we really shouldn't let them leak from object to object either, but for now we're letting g.attributes_ leak outside. (Dan, is that OK here?)

Please see http://dygraphs.com/changes.html, which describes lint and testing. (I have to update the testing doc since there are easier ways to run tests now.)

daminetreg · 2013-01-31T16:15:31Z

Thank you kberg for the comment, sorry that the diff you commented is now outdated, I messed up a push, that lead to the need of rewriting the history of the pull-request. Really sorry for this. ;)

We changed the indentation in respect to jslint, and fixed the code a little, but we don't understand about the proposal to use g.attributes_.getForSeries("stepPlot", e.setName);, because as far as we understand g.getOption calls exactly this. Are we missing something here ?

Also we figured out that passing the seriesName directly will do what we intend since getOption will return the global value if none is defined for the given series. (see dygraph.js line 563 - 566)

danvk · 2013-02-01T17:25:51Z

Please add either a test or an example of this being used. You can read about testing procedures at dygraphs.com/changes.html. At the very least, please add an example of a chart with both a stepPlot series and a non-stepPlot series to tests/steps.html.

stackedGraph and fillGraph options

eichsjul · 2013-02-07T10:26:54Z

Thanks for the feedback.
This is our new pull request try :-)
We rebased to your current version, added examples, added tests and fixed a couple of bugs we still had.
Now the stepPlot feature per series is fully functional for all plotting styles (errorBar,customBar,stacked,filled,line).
We had to partly rewrite the fillPlotter because it was based on the assumption that the previous series had the same line style as the current one. We added prevStepPlot and changed some if / else clauses. We have the feeling that it is now even clearer what the fill plotter does.

kberg · 2013-02-07T13:21:13Z

auto_tests/tests/stepPlot_per_series.js

+  document.body.innerHTML = "<div id='graph'></div>";
+};
+
+var _origFunc = Dygraph.getContext;


This is a global variable which might conflict with others. I suggest you use stepTestCase.origFunc

We have based this behavior on the following tests:

custom_bars.js
error_bars.js
missing_points.js
simple_drawing.js
to_dom_coords.js

which all use the same global variable. We're not sure what to do now since the same would apply to all of these tests. Any idea?

Take care of yours, and I'll fix it for all the others.

Also note that the standard is to have the test case name capitalized. Can you tweak that too, please?

You know what? Don't worry about the test name. There are enough that are different. Maybe I'll go fix them historically, but no big deal.

We will change the test name and also change the file name to lower case. We will also adapt the global var for the orginal getContext function for our text. thx

kberg · 2013-02-07T13:25:28Z

When I run this locally I get two test failures when I run ./test.sh

2 test(s) failed:
stepPlot_per_series.testMixedModeStepAndLineErrorBars failed.
stepPlot_per_series.testMixedModeStepAndLineStackedAndFilled failed.

and get the following when running on OSX/Chrome

Running testMixedModeStepAndLineStackedAndFilled fake-jstestdriver.js:101
Error
fake-jstestdriver.js:88
AssertError: Can't find a line drawn between 0,320 and 0,305.4545454545455 with attributes {}
at fail (file://localhost/Users/kberg/git/x/auto_tests/lib/Asserts.js:22:13)
at Object.CanvasAssertions.assertLineDrawn (file://localhost/Users/kberg/git/x/auto_tests/tests/CanvasAssertions.js:87:3)
at stepTestCase.testMixedModeStepAndLineStackedAndFilled (file://localhost/Users/kberg/git/x/auto_tests/tests/stepPlot_per_series.js:157:22)
at TestCase.testCase.runTest (file://localhost/Users/kberg/git/x/auto_tests/misc/fake-jstestdriver.js:84:10)
at TestCase.testCase.runAllTests (file://localhost/Users/kberg/git/x/auto_tests/misc/fake-jstestdriver.js:102:25)
at processVariables (file://localhost/Users/kberg/git/x/auto_tests/misc/local.html?testCaseName=stepPlot_per_series&command=runAllTests:135:22)
at file://localhost/Users/kberg/git/x/auto_tests/misc/local.html?testCaseName=stepPlot_per_series&command=runAllTests:248:1 fake-jstestdriver.js:90
Running testMixedModeStepAndLineErrorBars fake-jstestdriver.js:101
Error
fake-jstestdriver.js:88
AssertError: Can't find a line drawn between 118.75,194.28571428571428 and 118.75,224.76190476190476 with attributes {}
at fail (file://localhost/Users/kberg/git/x/auto_tests/lib/Asserts.js:22:13)
at Object.CanvasAssertions.assertLineDrawn (file://localhost/Users/kberg/git/x/auto_tests/tests/CanvasAssertions.js:87:3)
at stepTestCase.testMixedModeStepAndLineErrorBars (file://localhost/Users/kberg/git/x/auto_tests/tests/stepPlot_per_series.js:293:22)
at TestCase.testCase.runTest (file://localhost/Users/kberg/git/x/auto_tests/misc/fake-jstestdriver.js:84:10)
at TestCase.testCase.runAllTests (file://localhost/Users/kberg/git/x/auto_tests/misc/fake-jstestdriver.js:102:25)
at processVariables (file://localhost/Users/kberg/git/x/auto_tests/misc/local.html?testCaseName=stepPlot_per_series&command=runAllTests:135:22)
at file://localhost/Users/kberg/git/x/auto_tests/misc/local.html?testCaseName=stepPlot_per_series&command=runAllTests:248:1 fake-jstestdriver.js:90
Running testMixedModeStepAndLineCustomBars fake-jstestdriver.js:101
{"testMixedModeStepAndLineFilled":true,"testMixedModeStepAndLineStackedAndFilled":false,"testMixedModeStepAndLineErrorBars":false,"testMixedModeStepAndLineCustomBars":true} fake-jstestdriver.js:105
Loaded 195 tests in 34 test cases

assertLineDrawn can be pretty tricky. Are the tests passing for you?

eichsjul · 2013-02-07T14:22:26Z

Yes we have tried to run the test multiple times on our Win7/Chrome and they all passed flawlessly. We have now rerun them and also get the 2 failures. This must be due to the merge we did with your latest commits. However we have no idea what might have caused the suddon failure. We will investigate the problem and try to fix it.

sorry for causing all these problems

kberg · 2013-02-07T14:28:55Z

No need to apologize. You're helping us! But I'm glad you're getting the same failures. That's better than you still having the tests pass. :)

file to lower case

Corrected tests for stepPlot per series

eichsjul · 2013-02-07T16:05:03Z

The tests are fixed and the test name is now adapted as well as the variable "origFunc".
We hope everything works for you too :-)

kberg · 2013-02-07T16:18:51Z

I have confirmed that the tests pass. Dan?

danvk · 2013-02-08T20:21:48Z

Looks great. I'm pulling this now, thanks for the change!

…c466de7733 Added support for stepPlot per-series

kberg reviewed Jan 31, 2013
View reviewed changes

eichsjul and others added 5 commits February 7, 2013 11:09

FEATURE: Added support for stepPlot per-series.

2f56cd4

BUG FIX: Fixed indentation in respect to the jslint rules.

de9a818

FEATURE: Added stepPlot option per series for all plotters.

0ce5b19

BUGFIX: stepPlot per series now also works correctly for the

104d87c

stackedGraph and fillGraph options

FEATURE: Added tests for stepPlot option per series

4fa81db

kberg reviewed Feb 7, 2013
View reviewed changes

eichsjul added 2 commits February 7, 2013 15:39

BUGFIX: Renamed test to capital case, fixed global var stuff, renamed

5546c2e

file to lower case

BUGFIX:

1654747

Corrected tests for stepPlot per series

danvk added a commit that referenced this pull request Feb 8, 2013

Merge pull request #204 from sauter-hq/55eb8485b406174f15dcee0e97abc4…

5d93f81

…c466de7733 Added support for stepPlot per-series

danvk merged commit 5d93f81 into danvk:master Feb 8, 2013

eichsjul deleted the 55eb8485b406174f15dcee0e97abc4c466de7733 branch February 14, 2013 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for stepPlot per-series #204

Added support for stepPlot per-series #204

eichsjul commented Jan 31, 2013

kberg Jan 31, 2013

daminetreg commented Jan 31, 2013

danvk commented Feb 1, 2013

eichsjul commented Feb 7, 2013

kberg Feb 7, 2013

eichsjul Feb 7, 2013

kberg Feb 7, 2013

kberg Feb 7, 2013

kberg Feb 7, 2013

eichsjul Feb 7, 2013

kberg commented Feb 7, 2013

eichsjul commented Feb 7, 2013

kberg commented Feb 7, 2013

eichsjul commented Feb 7, 2013

kberg commented Feb 7, 2013

danvk commented Feb 8, 2013

Added support for stepPlot per-series #204

Added support for stepPlot per-series #204

Conversation

eichsjul commented Jan 31, 2013

kberg Jan 31, 2013

Choose a reason for hiding this comment

daminetreg commented Jan 31, 2013

danvk commented Feb 1, 2013

eichsjul commented Feb 7, 2013

kberg Feb 7, 2013

Choose a reason for hiding this comment

eichsjul Feb 7, 2013

Choose a reason for hiding this comment

kberg Feb 7, 2013

Choose a reason for hiding this comment

kberg Feb 7, 2013

Choose a reason for hiding this comment

kberg Feb 7, 2013

Choose a reason for hiding this comment

eichsjul Feb 7, 2013

Choose a reason for hiding this comment

kberg commented Feb 7, 2013

eichsjul commented Feb 7, 2013

kberg commented Feb 7, 2013

eichsjul commented Feb 7, 2013

kberg commented Feb 7, 2013

danvk commented Feb 8, 2013