Stacked Area Chart 'expanded' highlights not updated on data change or series toggling

[PeterVermont]

If I change the data for the chart when in expanded mode the highlighted points will still be for the initial data. The text percentages are correct though. I believe that 'point.display.y' in stackedAreaChart.js line 422 is not being updated.

Here is the initial data:

And here it is after changing the data:

[PeterVermont]

The bug is even easier to see by just removing one of the data groups by single clicking in the legend. In the picture below I have clicked (removed) the data for the largest category Home:

[PeterVermont]

Replicated the bug in the Nvd3 examples:

When I toogle off the large series 'Information Technology' you can see that the highlights are still showing the previous locations:

[PeterVermont]

Note: if you switch to either Stream or Stacked and then back to Expanded the highlighted points are updated to the correct locations

[liquidpele]

Hmm, this might be related to https://github.com/novus/nvd3/issues/1777 too

[PeterVermont]

Found the problem!

Point positions are not being updated because the data ids are unchanged and therefore the enter selection code does not get run even though the position needs to be updated.

The issue is that 'expanded' points are odd in that their attributes such as position can change even though their id does not.

Potential fixes (although I am not great at d3):

1. - Include the y-position of the point as part of each points data key.
2. -Alternatively, move the translation into the update selection rather than the enter selection.

For now, since I did not want to edit my nvd3 so I work around the bug by removing the points when I get a click in the legend which will cause the points to be in the enter selection and terefore have their position updated.

chart.legend.dispatch.on('legendClick', function (e) {
 d3.select(chart.container).selectAll('path.nv-point').remove();
});

Relevant code is in scatter.js starting at line 427:

            points.enter().append('path')
                .attr('class', function (d) {
                    return 'nv-point nv-point-' + d[1];
                })
                .style('fill', function (d) { return d.color })
                .style('stroke', function (d) { return d.color })
                .attr('transform', function(d) {
                    return 'translate(' + nv.utils.NaNtoZero(x0(getX(d[0],d[1]))) + ',' + nv.utils.NaNtoZero(y0(getY(d[0],d[1]))) + ')'
                })
                .attr('d',
                    nv.utils.symbol()
                    .type(function(d) { return getShape(d[0]); })
                    .size(function(d) { return z(getSize(d[0],d[1])) })
            );
            points.exit().remove();

[ThomasBrierley]

This was also a regression caused by my optimisation but with a separate pruning dependency to #1777

The issue was not the IDs changing. The getCache function can handle the series changing IDs because it uses the series ID as the key to the cache object. Instead it seems that the stacked area chart uses numerical keys instead of x, y for position as most models do.

If anyone is aware of any other alternative point position keys or other unusual models where point position is affected by other keys it would be good to know. Currently point update pruning is determined by chart dimensions and point x, y, 0, 1 values.

[PeterVermont]

I did not say the data ids were changing. In fact it is that the data ids remained the same but the positions changed. With the data ids not changed the code in the enter selection is not re-run.

If the code for the translation was in the update selection (which automatically always includes the enter selection) the positions would correctly be changed.

Unfortunately, if in the update selection, the translations would be applied all the time for all chart types.

I don't know the code but the essential issues seems to be that expanded positions are done differently than stream or stacked. While all three make the data series positions dependent on each other stacked and stream actually change the data point locations whereas expanded does on-the fly recalculation.

[ThomasBrierley]

Hi @PeterVermont Sorry I must have miss understood what you meant. Thank's for the bug report, which along with input from @liquidpele revealed an important flaw in an optimisation I made to this code. It's all fixed now on this PR: #1817 I hope I didn't cause you too much pain.

This was a regression so if you need the fix soon you could use a version older than v1.8.4 or wait for the next patch which I believe should be soon.

If the code for the translation was in the update selection (which automatically always includes the enter selection) the positions would correctly be changed.

Unfortunately, if in the update selection, the translations would be applied all the time for all chart types.

Although the behaviour might suggest otherwise, the translation code is and was in the update selection. Before the performance disadvantage that you outline was apparent with large dynamically updated data-sets which were not interactively usable.

The "getDiffs" filter in the update selections are culling the unchanged points which allows the update to be substantially faster, the bug was caused by the getDiffs function using a potentially different method to get point attributes for charts that override the default. (details in the PR).

[PeterVermont]

Hi @ThomasBrierley,

I see you are correct that there is a translation applied to the update selection. I was fooled because the translation is ALSO applied on the enter() (without the diffs filter) so I did not look down further.

Thanks for taking the time to explain what was going on.

There are a couple small optimizations I can see: • It is not necessary for the translation and the symbol code to be both in the enter and the update since the enter selection is appended to the update selection. This will work because getDiffs will not find the cache entry for new points and therefore they will pass the filter. • The data filter using scaleDiffs is unnecessary since it includes all points.

Below is my version of the section of relevant code in case it is helpful to you:

points.enter().append('path').attr('class', function (d) {
        return 'nv-point nv-point-' + d[1];
    }).style('fill', function (d) {
        return d.color
    }).style('stroke', function (d) {
        return d.color
    })
    //                .attr('transform', function(d) {
    //                    return 'translate(' + nv.utils.NaNtoZero(x0(getX(d[0],d[1]))) + ',' + nv.utils.NaNtoZero(y0(getY(d[0],d[1]))) + ')'
    //                })
    //                .attr('d',
    //                    nv.utils.symbol()
    //                    .type(function(d) { return getShape(d[0]); })
    //                    .size(function(d) { return z(getSize(d[0],d[1])) })
    //            )
;
points.exit().remove();
groups.exit().selectAll('path.nv-point').watchTransition(renderWatch, 'scatter exit').attr('transform', function (d) {
    return 'translate(' + nv.utils.NaNtoZero(x(getX(d[0], d[1]))) + ',' + nv.utils.NaNtoZero(y(getY(d[0], d[1]))) + ')'
}).remove();

var filteredPoints = scaleDiff ? points : points.filter(function (d) {
    return getDiffs(d, 'x', 'y');
});

filteredPoints.watchTransition(renderWatch, 'scatter points').attr('transform', function (d) {
    //nv.log(d, getX(d[0],d[1]), x(getX(d[0],d[1])));
    return 'translate(' + nv.utils.NaNtoZero(x(getX(d[0], d[1]))) + ',' + nv.utils.NaNtoZero(y(getY(d[0], d[1]))) + ')'
});

filteredPoints = scaleDiff ? points : points.filter(function (d) {
    return getDiffs(d, 'shape', 'size');
});

filteredPoints.watchTransition(renderWatch, 'scatter points').attr('d', nv.utils.symbol().type(function (d) {
    return getShape(d[0]);
}).size(function (d) {
    return z(getSize(d[0], d[1]))
}));

[ThomasBrierley]

Great! removal of the duplicate translation and symbol code should make a nice speedup because it's doubling the DOM access for new points :) it's also just not good to have literally duplicate code anyway.

I suspect moving the filter would have less effect unfortunately, I get what you are trying to achieve though: In theory it should save n calls of the encapsulating anonymous function and anything that happens inside of the D3 filter code... Although in reality i'm pretty sure that will not happen due to JavaScript optimising compilers (easy to determine that scaleDiff is not changing and is true for all). But I don't see the harm in your method, it definitely can't add any overhead.

More to the point though is that if scaleDiff or sizeDiff are true then it's kinda game over because the DOM node access outweighs all other processing in JavaScript, that's basically all that the point culling is trying to avoid, it actually adds some overhead, extra point function calls and lookups for all points.

Beyond abandoning stateful DOM based graphing I think this could be avoided if the points were set in axis based coordinate space, that way changes to chart and axis size would naturally rescale that coordinate space without having to access each point node... but there are issues with that approach too and it's beyond the scope of nvd3 really, I was just going after low hanging fruit :D

[ThomasBrierley]

@PeterVermont Do you want me to add your changes into my PR or are you going to do another PR? (Note those lines have changed a bit in my PR so it should be based on the new code once it's finalised)

[PeterVermont]

Please make whatever changes you think make sense. I have never done a PR on public projects!

…………………………………………….. PETER ANDREWS Senior Software Engineer

RSG 55 Railroad Row | White River Junction, VT 05001 o 802.359.6411 | m 802.356.3141 www.rsginc.comhttp://www.rsginc.com/ ……………………………………………..

From: Thomas Brierley [mailto:notifications@github.com] Sent: Wednesday, September 21, 2016 7:13 AM To: novus/nvd3 nvd3@noreply.github.com Cc: Peter Andrews peter.andrews@rsginc.com; Mention mention@noreply.github.com Subject: Re: [novus/nvd3] Stacked Area Chart 'expanded' highlights not updated on data change or series toggling (#1814)

@PeterVermonthttps://github.com/PeterVermont Do you want me to add your changes into my PR or are you going to do another PR?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/novus/nvd3/issues/1814#issuecomment-248581152, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA2Rkd5tgwswyE6GTo3oZDPAdnFjv2A1ks5qsRFMgaJpZM4J-KOs.

[liquidpele]

https://github.com/novus/nvd3/pull/1817 might have fixed, can someone verify lastest master?

[PeterVermont]

I can no longer replicate the issue I saw at: http://nvd3.org/examples/stackedArea.html

Nice job!