Datahandler and Unified Data Format

[davideberlein]

Hey Dan and Robert,

this is the pull request for my proposal:
https://docs.google.com/document/d/1IWmX4oDbQbVtUoRNzSRG3yMpoBQ7LseVCQhGnuOZz_A/edit?pli=1#

Lint runs without any issues and so do all of the tests. (Including additional ones I added)

As you can see I have done some performance tweaks and this solution is now is faster than the current version in every aspect.
This implementation may still be extended and changed here and there but it is fully functional and introduces the new structure which fixes some bugs and is a great benefit for future feature development, at least in my point of view.
I would really appreciate if we could think about pulling it in the near future as an internal change and only add external options later when you have the feeling that everything is the way it should be.

The problem for me in the moment is that I have to merge every changes you do on the master and this is quite a bit of work. I think my proposal is in the right stadium to be integrated and further developed on your code base.

What do you think?

Best Regards

David

[davideberlein]

Hey, I've done all the changes you requested (except the callback in DygraphLayouts). I tried to clean all the nits but as always I probably forgot something...

[danvk]

why are you removing this?

[davideberlein]

Because Robert said so ^^ .. at least that's what I thought he was saying, see:
#249 (diff)

[kberg]

My mistake: I meant that you had refactored it, seemingly to remove whitespace, and I wanted the file out of the pull request since it was a whitespace-only change.

[davideberlein]

Oh okay ;-) sorry for that.. I readded the line now.

[davideberlein]

Hey, thanks for the feedback, I'll look into this on monday

[danvk]

Hi David, any updates?

[davideberlein]

Hey, I refactored the JSDoc and installed the closure compiler to verify my changes.
Its still complaining that it doesn't know some types like Dygraphs or DygraphOptions. However I don't know how I can fix this.

[danvk]

Hi David -- we haven't forgotten about this! We're going to pull it in soon for dygraphs version 1.1.0. We're still hashing out exactly what our branching strategy is for dygraphs releases, which is why we haven't pulled it yet.

[danvk]

Hi David,

Good news -- I've pulled your change! Given the long history, I decided to pull it as a squash commit. Here it is:
a49c164

This should go out as part of the dygraphs 1.1.0 release. We may make some changes to address the remaining comments in this pull request (particularly the questions around Dygraph.DataHandlers.registerHandler), but we're happy to get this in.

Thanks for sticking with it -- and sorry this took so long!

[danvk]

Here's a bit of cleanup. Feel free to comment on the commit.
3ea41d8

[kberg]

I've discovered a limitation with this design. Specifically, let's assume that I want to make "wilsonInterval" vary per data series. Or sigma, or logscale (which will vary by axis, and not series, but allow me to not bother making that distinction any further, it by and large applies here.)

$ grep options.get *.js
bars-custom.js: var logScale = options.get('logscale');
bars-error.js: var sigma = options.get("sigma");
bars-error.js: var logScale = options.get('logscale');
bars-error.js: var sigma = options.get("sigma");
bars-fractions.js: var sigma = options.get("sigma");
bars-fractions.js: var logScale = options.get('logscale');
bars-fractions.js: var sigma = options.get("sigma");
bars-fractions.js: var wilsonInterval = options.get("wilsonInterval");
default-fractions.js: var logScale = options.get('logscale');
default.js: var logScale = options.get('logscale');

All these uses of options.get would need either the axis or series name being operated upon to know whether these options apply, which means using getForSeries instead of get.

The wilson interval is computed in this function:

FractionsBarsHandler.prototype.rollingAverage = function(originalData, rollPeriod, options) {

which has access to neither the series index or the series name.

Fortunately, this seems to be fairly centralized.

Here are two proposals:

1. change the API just a bit by adding the series name to extractSeries, rollingAverage, getExtremeYValues, all of which currently accept options.
2. Instead of supplying an instance of DygraphOptions, supply a closure around the options object so that it applies to the series being operated on. There are two problems with this, one simple and one nuanced. The simple problem is the change in the Closure-compatible jsdoc; the thing wouldn't be a DygraphsOptions object. The nuanced problem is described below, with a solution. But skip it unless you want to cry from mental exhaustion.

DygraphOptions.getForSeries looks for an option in the series, and then the axis, and then globally, so for instance, you can set the point size for an entire axis by setting an option:

axes : { y2 : { pointSize : 6 }}.

Any for all series on the y2 axis, the points will be drawn with radius 6. (Or is that diameter? Never mind.)

So what would happen if we called

options.getForSeries("logscale", seriesName)?

Well, if someone was foolish enough to set logscale in their series, and not in their axis, like so:

series : {
foo : { axes : y2, logscale : false }
},
axes : {
y2 : { logscale : true }
}

they would be told that logscale is false, when, no, that's bad!

Get it? So, that doesn't mean we can't use the closure idea, it just means we have to make option processing better. Solution? We do that by throwing errors when setting per-series options on things like "logscale" and "includezero".