Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix non-time tick generation for changed data. #76

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

dbishop commented Sep 16, 2012

There was a bug where non-time axes (i.e. the default "base 10
behavior") would assign a function to axis.tickGenerator which used
axis.tickSize to generate ticks. But nothing would recalculate
axis.tickSize or assign a new function to axis.tickGenerator when new
data was loaded and the plot redrawn. This would cause way to many
ticks to be drawn when a plot was reused to render data with a greater
delta (axis.max - axis.min).

This bug arose because there are two ways of handling axis tick
generation. The default "base 10" fall-back, and the time plugin.

The time plugin's ticks are not vulnerable to the above bug because it
reassigns to axis.tickGenerator upon every data load, and most of its
calculations are done inside the tickGenerator function.

This patch fixes the above bug by implementing the default "base 10"
behavior as an inline plugin (to maintain backard compatibility)
analogous to the time plugin. This new plugin is used when the
axis.mode is "base10" or left undefined.

Now axis tick generation is consistently handled by plugins, and if no
specific mode is specified, the "base10" plugin is used.

@dbishop dbishop Fix non-time tick generation for changed data.
There was a bug where non-time axes (i.e. the default "base 10
behavior") would assign a function to axis.tickGenerator which used
axis.tickSize to generate ticks.  But nothing would recalculate
axis.tickSize or assign a new function to axis.tickGenerator when new
data was loaded and the plot redrawn.  This would cause *way* to many
ticks to be drawn when a plot was reused to render data with a greater
delta (axis.max - axis.min).

This bug arose because there are two ways of handling axis tick
generation.  The default "base 10" fall-back, and the time plugin.

The time plugin's ticks are not vulnerable to the above bug because it
reassigns to axis.tickGenerator upon every data load, and most of its
calculations are done inside the tickGenerator function.

This patch fixes the above bug by implementing the default "base 10"
behavior as an inline plugin (to maintain backard compatibility)
analogous to the time plugin.  This new plugin is used when the
axis.mode is "base10" or left undefined.

Now axis tick generation is consistently handled by plugins, and if no
specific mode is specified, the "base10" plugin is used.
a94d810

dbishop commented Sep 16, 2012

Here's the related flot Issue I filed: http://code.google.com/p/flot/issues/detail?id=754

@dnschnur dnschnur was assigned Sep 21, 2012

@markrcote markrcote was assigned Dec 3, 2012

Cool, I imagine that this could also be fixed by just moving the calculation of axis.tickSize into the tickGenerator() function, but I like the modularity involved in putting it into a plugin. The patch has bitrotted but I'll clean it up.

@markrcote markrcote added a commit to markrcote/flot that referenced this pull request Dec 28, 2012

@markrcote markrcote Unbitrotted pull request #76. e30d3c4
Contributor

markrcote commented Dec 28, 2012

I have pushed an unbitrotted version of this pull request to the base10-plugin branch in my repo (plus an extra fix for categories example as described below).

However, I don't think that this patch is the right approach. The main problem is that it can break existing plugins, such as the categories plugin.

The categories example is broken because the categories plug-in is triggered when the axis mode is set to "categories". This isn't recognized by the base10 plugin, and the categories plugin doesn't actually define a tick generator but just changes around the raw data, so the "How is axis.tickGenerator not a function?!" exception gets raised. I fixed this by implementing a forceBase10 option, but I don't really like it, and it will require similar changes in any existing plugins that do similar things (none packaged with flot, but there might be others out there).

The turning-series.html example is also broken, in an odd way: when you turn off all the series except the first (USA), there is an exception: "TypeError: colors[s.color] is undefined" (on line 486 of jquery.flot.js in my branch). I haven't investigated this yet.

When digging into all this, though, I realized that this is a known issue, arguably by design. The API doc states, for setData(), "You can use this function to speed up redrawing a small plot if you know that the axes won't change" (emphasis added). This is not an issue if $.plot() is used instead of setData() and draw().

Admittedly, this patch does improve the panning and zooming example (navigate.html) when you zoom out by removing some y-axis ticks. However I imagine that the nagivate plugin could itself be fixed.

So, in light of the breakage and the fact that this was apparently never intended to work, I don't think I will accept this pull request as is. Feel free to make a smaller change that won't break other plugins, but I am of the opinion that it isn't really necessary, since there is already a way around this problem. I'll get a second opinion from dnschnur.

dbishop commented Dec 29, 2012

Sorry for the delay in responding to your comments (and thanks for taking a look at this and giving me great feedback!).

I'll have to load all the appropriate context back into my head and test using $.plot() instead of setData() and draw() for our use-case. If that doesn't work well, I'll see if I can cook up a fix which at least doesn't break other existing or possibly-created-in-the-future plugins.

It's been a while since I worked on this, but I remembered the axis code being difficult to work with, and the two ways of handling axis ticks ("built-in" for default or maybe "time" implemented as a plug-in) to be awkward and inconsistent. I think the problems this patch has with other plugins is just a different flavor of the same design-smell. So maybe there's a redesign I couldn't put my finger on back then which allows plugins to not break each other, while still all getting correct behavior, even in the face of changing data; surely that's a common use-case for dynamic graphs where UI elements allow jumping around in time.

I also need to look at the categories plugin to see how it works and how a graph with no axis.tickGenerator function works.

Contributor

markrcote commented Dec 29, 2012

We want to do a lot of work with plug-ins for the 0.9 release, so we can take a look at this at the same time. It is definitely confusing. I guess one problem is the use of the "mode" option--it has no well-defined meaning. It seems to vaguely mean "do something special with the ticks", but that may mean replacing the tick generator (or generating the ticks themselves), as the time plug-in does, or just modifying the plot data and using the "built-in" base-10 generator, as the categories plug-in does. The problem with moving the default base-10 representation to a plug-in is that it can't really be used as a default anymore--it's on par with every other plug-in, and, as you say, can be clobbered by others.

Btw you can check the examples for uses of $.plot() to replot data, such as examples/ajax.html.

So I will close this request, but feel free to submit another patch, or start a discussion on the mailing list. I'm sure we'll be discussing plug-in architecture there before too long.

@markrcote markrcote closed this Dec 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment