Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[discreteBarChart] Fix y axis ticks number calculation. #2105

Merged
merged 2 commits into from
Mar 1, 2018
Merged

[discreteBarChart] Fix y axis ticks number calculation. #2105

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4105ea7
[discreteBarChart] Fix y axis ticks number calculation.
ovvn Sep 27, 2017
e8432bd
[discreteBar] Remove yAccessor param. [calcTicksY] Re-use getY fn.
ovvn Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/models/discreteBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like yAccessor should replace getY

, forceY = [0] // 0 is forced by default.. this makes sense for the majority of bar graphs... user can always do chart.forceY([]) to remove
, color = nv.utils.defaultColor()
, showValues = false
Expand Down Expand Up @@ -222,6 +223,7 @@ nv.models.discreteBar = function() {
showValues: {get: function(){return showValues;}, set: function(_){showValues=_;}},
x: {get: function(){return getX;}, set: function(_){getX=_;}},
y: {get: function(){return getY;}, set: function(_){getY=_;}},
yAccessor: {get: function(){return yAccessor;}, set: function(_){yAccessor=_;}},
xScale: {get: function(){return x;}, set: function(_){x=_;}},
yScale: {get: function(){return y;}, set: function(_){y=_;}},
xDomain: {get: function(){return xDomain;}, set: function(_){xDomain=_;}},
Expand Down
2 changes: 1 addition & 1 deletion src/models/discreteBarChart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ nv.models.discreteBarChart = function() {
if (showYAxis) {
yAxis
.scale(y)
._ticks( nv.utils.calcTicksY(availableHeight/36, data) )
._ticks( nv.utils.calcTicksY(availableHeight/36, data, discretebar.yAccessor()) )
.tickSize( -availableWidth, 0);

g.select('.nv-y.nv-axis').call(yAxis);
Expand Down
30 changes: 26 additions & 4 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,11 +450,33 @@ nv.utils.calcTicksX = function(numTicks, data) {


/*
returns number of ticks to actually use on Y axis, based on chart data
*/
nv.utils.calcTicksY = function(numTicks, data) {
// 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, yAccessor) {
if (yAccessor) {
// find max number of values from all data streams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is incorrect

var numValues = 1;
for (var i=0; i < data.length; i += 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code fails linting guidelines

var values = data[i] && data[i].values ? data[i].values : [];
var maxValue;
for (var j=0; j < values.length; j += 1) {
maxValue = values[j] && values[j][yAccessor] ? values[j][yAccessor]: 0;
numValues = maxValue > numValues ? maxValue : numValues;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nv.log("Requested number of ticks: ", numTicks);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debugging code should be removed

nv.log("Calculated max values to be: ", numValues);
// make sure we don't have more ticks than values to avoid duplicates
numTicks = numTicks > numValues ? numValues - 1 : numTicks;
// make sure we have at least one tick
numTicks = numTicks < 1 ? 1 : numTicks;
// make sure it's an integer
numTicks = Math.floor(numTicks);
nv.log("Calculating tick count as: ", numTicks);
return numTicks;
} else {
return nv.utils.calcTicksX(numTicks, data);
}
};


Expand Down