subhrajitroy / Flot-plugin

This plugin allows users to define constraints on data ,so that values that satify a given constraint will appear in the graph in different colors.
13 stars 9 forks source link

Threshold system is needlessly complex #9

Open jemenake opened 11 years ago

jemenake commented 11 years ago

After trying to figure out why I couldn't get the coloring to work the way I wanted, and delving through the plugin code, I have a few comments: getSortedConstraints() assumes that all constraints have the same "type" of threshold in evaluate() (in that they're either all "return y > threshold" or all "return y < threshold"). If you mix these types in your constraints, you get unpredictable behavior because _getSortedConstraints() uses evaluate() to determine whether to calculate "range" with Math.abs(constraint.threshold - min) or with Math.abs(max - constraint.threshold).

if(constraint.evaluate(min,constraint.threshold)){
   range = Math.abs(constraint.threshold - min);
}else{
   range = Math.abs(max - constraint.threshold);
}
thresholdRanges.push({constraint:constraint,range:range});

So, it's noting how far the threshold is from either the top or bottom of the plot. But, if you mix your ">" and "<" values in your constraints, the values of "range" aren't consistent and the constraints aren't sorted properly.

What I think would be a better plan would be to not do any sorting of the constraints. Trust the caller. Evaluate the constraints in the order in which they appear in the array and take the first one which matches, if any.

At that point, it would be tempting to toss "threshold" out of the constraints structure, entirely; Thresholds would be hard-coded into the evaluate() function instead of hard-coded to a "threshold" value. And then the structure ends up looking a little like a series of if/ifelse or case statements:

var constraints = [
    {  // over 10 gets green
        color: "rgb(0,255,0)",
        evaluate: function(y) { return y > 10; }
    },
    {  // otherwise, over 1 gets yellow
        color: "rgb(250,229,75)",
        evaluate: function(y) { return y > 1; }
    },
    {  // otherwise, over 0 gets red (change to "return true" to have *all* remaining points get colored red)
        color: "rgb(255,0,0)",
        evaluate: function(y) { return y > 0; }
    }
];

the problem here, is that, without threshold values, you wouldn't know where, in the line between two points, to break it into two colors. I think the best option might be something like:

var constraints = [
    {  // red from 10 up to the top of the plot
        color: "rgb(0,255,0)",
        threshold: 10
    },
    {  // above 5 gets yellow
        color: "rgb(250,229,75)",
        threshold: 5
    },
    {  // everything below is green
        color: "rgb(255,0,0)",
        threshold: null   // If this were a value, then points below it would be the default plot color
    }
];

By looking at the code, I suspect that's the way you had it, originally, until someone wanted to put an upper limit on the top color. The way you could limit the top color is by adding a "null" color before it:

var constraints = [
    {  // beyond 20, use the default color
        color: null,
        threshold: 20
    },
    {  // red from 10 up to the top of the plot
        color: "rgb(0,255,0)",
        threshold: 10
    },
    ...

If you do it this way, I think you might be able to ditch almost 1/2 of the code. I don't think you'll need QuickSort(), or _findMaxAndMin(), or _getSortedConstraints().