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.

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

Boxplot - null points cause incorrect legend behaviour #4939

Closed
sebastianbochan opened this Issue Jan 20, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@sebastianbochan
Contributor

sebastianbochan commented Jan 20, 2016

Steps to reproduce:

  1. http://jsfiddle.net/jvufyfrf/
  2. Click on the Series 2
  3. As a result both series are hidden.

This is caused by null points inside array, when we have all values, legend works properly:
http://jsfiddle.net/2gb8k75r/

@sebastianbochan sebastianbochan added the Bug label Jan 20, 2016

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Jan 20, 2016

Contributor

Workaround:
Use [null, null, null, null, null] instead of null, demo: http://jsfiddle.net/jvufyfrf/2/

Internal note:
I think this checking is wrong. There should be defined(y[j]).

Extra note:
Why not using defined(y) && (!yAxis.isLog || (y.length || y > 0)) also here.

Contributor

pawelfus commented Jan 20, 2016

Workaround:
Use [null, null, null, null, null] instead of null, demo: http://jsfiddle.net/jvufyfrf/2/

Internal note:
I think this checking is wrong. There should be defined(y[j]).

Extra note:
Why not using defined(y) && (!yAxis.isLog || (y.length || y > 0)) also here.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 21, 2016

Collaborator

For the records, it is bad practice adding nulls to make the series start at a later X value. Instead, you should either

Collaborator

TorsteinHonsi commented Jan 21, 2016

For the records, it is bad practice adding nulls to make the series start at a later X value. Instead, you should either

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 21, 2016

Collaborator

@pawelfus I'm a bit concerned that using defined will have performance impacts, since this is an operation that is carried of several times for each point. Perhaps it is better to catch the bug earlier, for example in the boxplot implementation.

Collaborator

TorsteinHonsi commented Jan 21, 2016

@pawelfus I'm a bit concerned that using defined will have performance impacts, since this is an operation that is carried of several times for each point. Perhaps it is better to catch the bug earlier, for example in the boxplot implementation.

@sebastianbochan sebastianbochan changed the title from Boxplot - null points cases incorrect legend behaviour to Boxplot - null points cause incorrect legend behaviour Jan 21, 2016

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Jan 21, 2016

Contributor

Of course, it will double the checks (comparison against null vs comparison against null and undefined). Maybe creating proper yData would be a way to go? Right now, when simple null is set as a point, then generated array (for boxplot) is [null, undefined, undefined, undefined, undefined], while should be [null, null, null, null, null] (I think?).

In that case, we need to improve logic here.

Like this:

each(pointArrayMap, function (key) {
    ret[key] = options;
});
Contributor

pawelfus commented Jan 21, 2016

Of course, it will double the checks (comparison against null vs comparison against null and undefined). Maybe creating proper yData would be a way to go? Right now, when simple null is set as a point, then generated array (for boxplot) is [null, undefined, undefined, undefined, undefined], while should be [null, null, null, null, null] (I think?).

In that case, we need to improve logic here.

Like this:

each(pointArrayMap, function (key) {
    ret[key] = options;
});
@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 22, 2016

Collaborator

Ouch, another performance sensitive test that has wide effect... If we don't find a more boxplot specific solution I suggest we leave it for now, as there are good workarounds.

Collaborator

TorsteinHonsi commented Jan 22, 2016

Ouch, another performance sensitive test that has wide effect... If we don't find a more boxplot specific solution I suggest we leave it for now, as there are good workarounds.

@wergeld

This comment has been minimized.

Show comment
Hide comment
@wergeld

wergeld Aug 24, 2016

I would also like to add that there is an issue if only one point is null valued in the array. In this example notice that in data2 series the "high" point is null and the rest of that data element does not show. If you change the null point to the "low" value then it doesnt render the low point but it also does not render the high point.

wergeld commented Aug 24, 2016

I would also like to add that there is an issue if only one point is null valued in the array. In this example notice that in data2 series the "high" point is null and the rest of that data element does not show. If you change the null point to the "low" value then it doesnt render the low point but it also does not render the high point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment