Warn if points are out of range #1050
Conversation
|
Let’s hope the extra if isn’t going to be a performance issue…
On Tue, 22 Oct 2024, Enguerrand de Ribaucourt wrote:
Note that may spew a lot of warnings if their are a lot of points.
How about warning only once? Perhaps just adding ((x<0)||(x>1)) to
a running counter, so we have the comparisons’ conditionals but no
extra conditional jump in the inner loop.
bye,
//mirabilos
|
|
@mirabilos Done, thanks a lot for your review! |
deribaucourt
force-pushed
the
master
branch
3 times, most recently
from
49f6836 to
d56f470
Compare
src/dygraph-layout.js
Outdated
|
|
||
| for (var j = 0; j < points.length; j++) { | ||
| var point = points[j]; | ||
|
|
||
| // Range from 0-1 where 0 represents left and 1 represents right. | ||
| point.x = DygraphLayout.calcXNormal_(point.xval, this._xAxis, isLogscaleForX); | ||
| outOfXBounds += ((point.x<0)||(point.x>1)); |
There was a problem hiding this comment.
nitpick (point.x < 0) || (point.x > 1), I had left out the spaces in the mail to convey the idea and not go over normal eMail line length, and the outer parenthesēs are redundant here
src/dygraph-layout.js
Outdated
| @@ -269,6 +271,14 @@ DygraphLayout.prototype._evaluateLineCharts = function() { | |||
| } | |||
| } | |||
| point.y = DygraphLayout.calcYNormal_(axis, yval, logscale); | |||
| outOfYBounds += ((point.y<0)||(point.y>1)); | |||
src/dygraph-layout.js
Outdated
| outOfYBounds += ((point.y<0)||(point.y>1)); | ||
| } | ||
|
|
||
| if (outOfXBounds > 0) { |
There was a problem hiding this comment.
I wonder whether this even makes sense for the x axis… or rather, when zooming in, some points are always going to be out of bounds, also for the y axis. Maybe the checks should be conditional on not having zoomed in? Unless they are already excluded then…
Perhaps we need to play around a bit with this.
There was a problem hiding this comment.
Interesting question!
If I zoom in, the warning is only displayed for 2 points (those that allow drawing the line going out of the edge it appears). So it looks like all other points are excluded.
The rest of the time, x axis warnings are really interesting. In my use case for instance, I needed them to debug a graph not being shown because the x axis was improperly configured.
I think we would be good with simply ignoring 2 points from the x axis out of bounds counter. It's not going to generate false negatives except for graphs with just 2 points. While providing protection for zooming.
Some users may improperly define the range of their axes, leading to points being drawn outside of the chart area. In the worst cases, if all points are out of range, the chart will not be drawn at all. Adding a warning can help users identify this issue and correct it. 2 x points are allowed in order to draw the edges of the chart when zoomed in.
Some users may improperly define the range of their axes, leading to points being drawn outside of the chart area. In the worst cases, if all points are out of range, the chart will not be drawn at all.
Adding a warning can help users identify this issue and correct it.
Note that may spew a lot of warnings if their are a lot of points.
Such a warning helped me debug the usage of netdata. See netdata/netdata-cloud#1048