[discreteBarChart] Fix y axis ticks number calculation. #2105
Conversation
|
+1 |
6 similar comments
|
+1 |
|
+1 |
|
+1 |
|
+1 |
|
+1 |
|
+1 |
|
Seems like unit tests would be a good idea given large changes to calcTicksY function. Also why the nv.log calls? |
src/utils.js
Outdated
| maxValue = values[j] && values[j][yAccessor] ? values[j][yAccessor]: 0; | ||
| numValues = maxValue > numValues ? maxValue : numValues; | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems super inefficient... so this is looking at every node on the chart? Surely there is a better way to chose a tick count...
There was a problem hiding this comment.
Let's see. Incoming data for the discreteBarChart always has the following format:
[
{
"values": [
{
"label": "Label1",
"value": 3,
"series": 0
},
{
"label": "Label2",
"value": 123,
"series": 0
},
{
"label": "Label3",
"value": 777,
"series": 0
},
...
]
}
]
So complexity would be O(n) where n = data[0].values.length. The linear complexity is obviously efficient and very fast.
There was a problem hiding this comment.
Just to clarify, the idea behind this pull request is to calculate y axis ticks for the discrete bar chart based on y values, not on x values as it is now https://jsfiddle.net/ovvn/g5qwwthx/1/embedded/result/. We've been using the fix for half a year on production and it works great for us.
There was a problem hiding this comment.
Yes, it's O(n), but for very large (millions) of points n is still sufficiently big to potentially cause problems in a browser. And that's not just worst case, it's every time because you always have to traverse the entire array. I see you're thinking about a simple bar chart, but this function is used by more than just that... if you want to override the function to calculate the tick count just for one type of chart, then I'd keep all the logic in the chart itself.
Also, what's the difference between getY and yAccessor options? Seems like that's overlapping functionality?
There was a problem hiding this comment.
Traversal in an array with millions of items is not a problem. What can be a problem is appending millions of elements into a browser's DOM, but that has nothing to do with the pull request. The functionality is not made for just one type of chart, it's generic and can be extended for other types as well. The difference between getY and yAccessor is that getY is a function and yAccessor is a string (y value property name). But that's a good point, don't remember why I made the property name to be passed explicitly. Think we can reuse the getY fn.
|
This appears to use the same logic as this PR, so linking them: #2028 |
src/models/discreteBar.js
Outdated
| @@ -15,6 +15,7 @@ nv.models.discreteBar = function() { | |||
| , y = d3.scale.linear() | |||
| , getX = function(d) { return d.x } | |||
| , getY = function(d) { return d.y } | |||
| , yAccessor | |||
There was a problem hiding this comment.
Seems like yAccessor should replace getY
src/utils.js
Outdated
| */ | ||
| nv.utils.calcTicksY = function(numTicks, data, yAccessor) { | ||
| if (yAccessor) { | ||
| // find max number of values from all data streams |
There was a problem hiding this comment.
If I'm not mistaken, this comment doesn't reflect what the code does. It doesn't find the max number of values, it finds the maximum y value in the entire dataset.
| */ | ||
| nv.utils.calcTicksY = function(numTicks, data, getY) { | ||
| if (getY) { | ||
| // find max number of values from all data streams |
| // currently uses the same logic but we can adjust here if needed later | ||
| returns number of ticks to actually use on Y axis, based on chart data | ||
| */ | ||
| nv.utils.calcTicksY = function(numTicks, data, getY) { |
There was a problem hiding this comment.
There should be unit tests for this function
| if (getY) { | ||
| // find max number of values from all data streams | ||
| var numValues = 1; | ||
| for (var i=0; i < data.length; i += 1) { |
There was a problem hiding this comment.
This code fails linting guidelines
| numValues = maxValue > numValues ? maxValue : numValues; | ||
| } | ||
| } | ||
| nv.log("Requested number of ticks: ", numTicks); |
There was a problem hiding this comment.
Debugging code should be removed
|
@liquidpele @ovvn I don't feel that this code was ready to be merged for the following reasons
I have left line comments in the PR pointing out these issues. Would be great to see a follow up commit from @ovvn addressing these points. |
|
|
@liquidpele |
Fixes #2104