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

`minPointLength` shifts all following values in waterfall #6087

Closed
hon2a opened this Issue Dec 5, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@hon2a

hon2a commented Dec 5, 2016

Expected behaviour

When I use minPointLength in a waterfall chart, it should force a minimum (visual) size of all columns, but it shouldn't shift the starting points of following columns.

Actual behaviour

Using minPointLength in a waterfall actually shifts all of the following columns, putting them in (sometimes wildly) different places than where they should be and possibly distorting the shape of the whole series to be completely misrepresentative of the data it visualizes.

Live demo with steps to reproduce

  • regular demo - note how the 90% column at the end is bigger than the 100% column at the beginning (it's also exhibiting some weird rendering bug making column bases shifted up, but that's not the concern of this ticket)
  • zero adjustments demo - this prevents me from making waterfall zero adjustments visible using minPointLength: 1

Use the input below the chart to quickly experiment with different minPointLength values.

Affected browser(s)

All of them, AFAICS. (I tested it in IE11, Chrome, and Firefox.)


I was hoping this ticket will get the issue resolved, but it's now supposedly fixed and the issue remains.

@znevrly

This comment has been minimized.

Show comment
Hide comment
@znevrly

znevrly Dec 6, 2016

+1 for fixing this.

znevrly commented Dec 6, 2016

+1 for fixing this.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Dec 6, 2016

Contributor

Thank you for reporting the issue!

  • regular demo:

That will be fixed of course. Previous ticket assumed values are noticeable smaller.

  • zero-adjustments demo:

I am open for a suggestions here. Minimum point length is applied for all points when height is lower than a set value. Of course, it will increate the last point - instead I suggest to use minPointLength ~ 0.0001. For example: http://jsfiddle.net/1qu32g9n/2/

In my opinion setting to high minPointLength is simply wrong, for example: http://jsfiddle.net/1qu32g9n/3/ - I don't see any logic to compromise: chart's height, bars height and actual values on the chart. Note: my second demo is exactly the same issue as "zero-adjustments", just shows the real issue here.

Contributor

pawelfus commented Dec 6, 2016

Thank you for reporting the issue!

  • regular demo:

That will be fixed of course. Previous ticket assumed values are noticeable smaller.

  • zero-adjustments demo:

I am open for a suggestions here. Minimum point length is applied for all points when height is lower than a set value. Of course, it will increate the last point - instead I suggest to use minPointLength ~ 0.0001. For example: http://jsfiddle.net/1qu32g9n/2/

In my opinion setting to high minPointLength is simply wrong, for example: http://jsfiddle.net/1qu32g9n/3/ - I don't see any logic to compromise: chart's height, bars height and actual values on the chart. Note: my second demo is exactly the same issue as "zero-adjustments", just shows the real issue here.

@pawelfus pawelfus self-assigned this Dec 6, 2016

@pawelfus pawelfus added the Bug label Dec 6, 2016

@hon2a

This comment has been minimized.

Show comment
Hide comment
@hon2a

hon2a Dec 6, 2016

My original zero-adjustments demo was misleading, because the data labels were obscuring the main point of the minPointLength use - presence of the columns. I've updated the data labels with tooltips and updated the link.


Using minPointLength lower than 1 does nothing for me, because the whole point of setting minPointLength: 1 is to make the zero values visible in the chart. Making it much higher than 1 doesn't really make sense in this context, but as you said, it does show the issue, which is IMO that

minPointLength is just a visualization property, it shouldn't affect data integrity.

While minPointLength can make a waterfall column higher than its value warrants, it shouldn't affect how the position of the next column is computed. It should be applied only after all of the column positions and lengths are computed and prolong the column in the direction based on the sign of its value (move column top up by default but move column bottom down for negative adjustments instead).

hon2a commented Dec 6, 2016

My original zero-adjustments demo was misleading, because the data labels were obscuring the main point of the minPointLength use - presence of the columns. I've updated the data labels with tooltips and updated the link.


Using minPointLength lower than 1 does nothing for me, because the whole point of setting minPointLength: 1 is to make the zero values visible in the chart. Making it much higher than 1 doesn't really make sense in this context, but as you said, it does show the issue, which is IMO that

minPointLength is just a visualization property, it shouldn't affect data integrity.

While minPointLength can make a waterfall column higher than its value warrants, it shouldn't affect how the position of the next column is computed. It should be applied only after all of the column positions and lengths are computed and prolong the column in the direction based on the sign of its value (move column top up by default but move column bottom down for negative adjustments instead).

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Dec 6, 2016

Contributor

Thanks for your feedback!

Then how about this use-case: http://jsfiddle.net/1qu32g9n/5/ - all values are positive, and moving column to the bottom doesn't make sense, right?

Isn't better to use tooltip.shared instead? In that case, tooltip will display regardless of the column shape, please take a look: http://jsfiddle.net/1qu32g9n/7/

Contributor

pawelfus commented Dec 6, 2016

Thanks for your feedback!

Then how about this use-case: http://jsfiddle.net/1qu32g9n/5/ - all values are positive, and moving column to the bottom doesn't make sense, right?

Isn't better to use tooltip.shared instead? In that case, tooltip will display regardless of the column shape, please take a look: http://jsfiddle.net/1qu32g9n/7/

@hon2a

This comment has been minimized.

Show comment
Hide comment
@hon2a

hon2a Dec 6, 2016

This mock demo shows how I'd expect the zero-adjustments waterfall to look. This mock demo shows how I'd expect your one-percent-adjustments to look. And these two show pretty much how I'd expect those charts to look if minPointLength was increased. (It actually isn't, because on columnrange the minPointLength expands the column in both directions, but close enough. And my one-directional expansion proposal might be subjective, so this might be completely accurate in fact.)

Tooltips don't really come into it anywhere. The problem is that ATM setting minPointLength messes up the visualization of the whole series, because the position of a column is computed based on how the previous column looks. Instead, the position should be computed purely based on the series' data and minPointLength should only affect the columns individually (not incrementally).

hon2a commented Dec 6, 2016

This mock demo shows how I'd expect the zero-adjustments waterfall to look. This mock demo shows how I'd expect your one-percent-adjustments to look. And these two show pretty much how I'd expect those charts to look if minPointLength was increased. (It actually isn't, because on columnrange the minPointLength expands the column in both directions, but close enough. And my one-directional expansion proposal might be subjective, so this might be completely accurate in fact.)

Tooltips don't really come into it anywhere. The problem is that ATM setting minPointLength messes up the visualization of the whole series, because the position of a column is computed based on how the previous column looks. Instead, the position should be computed purely based on the series' data and minPointLength should only affect the columns individually (not incrementally).

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Dec 15, 2016

Contributor

Thank you for the explanation. And my apologies for a delay.

There is one problem with a solution you propose. For example this demo: http://jsfiddle.net/1qu32g9n/8/ - the first point will rendered below zero which is certainly a a misleading representation.

I am worried, that one way or another, we will break data integrity anyway - that's what minPointLength actually does.

minPointLength should only affect the columns individually (not incrementally)

We can change that, but it will break the idea in the waterfall chart: shouldn't every point start at a position when previous ends? Just to be on the same page: of course you are right, the current solution is not a perfect one as the last (sum) column may be very misleading in a certain cases.

My point with shared tooltip is that minPointLength is not the best idea (imho) to use in a waterfall series. Instead it's better to use tooltip to display values when those are equal to zero.

Contributor

pawelfus commented Dec 15, 2016

Thank you for the explanation. And my apologies for a delay.

There is one problem with a solution you propose. For example this demo: http://jsfiddle.net/1qu32g9n/8/ - the first point will rendered below zero which is certainly a a misleading representation.

I am worried, that one way or another, we will break data integrity anyway - that's what minPointLength actually does.

minPointLength should only affect the columns individually (not incrementally)

We can change that, but it will break the idea in the waterfall chart: shouldn't every point start at a position when previous ends? Just to be on the same page: of course you are right, the current solution is not a perfect one as the last (sum) column may be very misleading in a certain cases.

My point with shared tooltip is that minPointLength is not the best idea (imho) to use in a waterfall series. Instead it's better to use tooltip to display values when those are equal to zero.

@hon2a

This comment has been minimized.

Show comment
Hide comment
@hon2a

hon2a Dec 15, 2016

There is one problem with a solution you propose. For example this demo: http://jsfiddle.net/1qu32g9n/8/ - the first point will rendered below zero which is certainly a misleading representation.

That's not what I proposed though, I proposed for each column to start where computed from the data and then be prolonged in the direction it's "pointing". IMO that's exactly the impact to be expected from the minPointLength property. Of course it will make the visualization different from the data on principle, but by breaking integrity I meant the impact on position of the following points that makes the shape of the whole series inconsistent with the data.

We can change that, but it will break the idea in the waterfall chart: shouldn't every point start at a position when previous ends?

First and foremost, the chart should correctly represent the data. What you describe is an invariant of the waterfall data. It doesn't mean that keeping this invariant in the visualization should take precedence over keeping it consistent with the data.

My point with shared tooltip is that minPointLength is not the best idea (imho) to use in a waterfall series. Instead it's better to use tooltip to display values when those are equal to zero.

I disagree. This issue points out a bug in waterfall rendering - minPointLength is used to transform the actual data, not just "length" of individual columns. Of course you could just say minPointLength is simply not supported for waterfall, but then it should be clearly stated in the documentation.

I don't see though, how the functionality I propose above is much different from how minPointLength is used in other types of series. And even if it instead behaved exactly like for the column range series (the midpoint position of each column would be calculated and then it'd grow in both directions), how is the result any more misleading in the waterfall than in the column range?

hon2a commented Dec 15, 2016

There is one problem with a solution you propose. For example this demo: http://jsfiddle.net/1qu32g9n/8/ - the first point will rendered below zero which is certainly a misleading representation.

That's not what I proposed though, I proposed for each column to start where computed from the data and then be prolonged in the direction it's "pointing". IMO that's exactly the impact to be expected from the minPointLength property. Of course it will make the visualization different from the data on principle, but by breaking integrity I meant the impact on position of the following points that makes the shape of the whole series inconsistent with the data.

We can change that, but it will break the idea in the waterfall chart: shouldn't every point start at a position when previous ends?

First and foremost, the chart should correctly represent the data. What you describe is an invariant of the waterfall data. It doesn't mean that keeping this invariant in the visualization should take precedence over keeping it consistent with the data.

My point with shared tooltip is that minPointLength is not the best idea (imho) to use in a waterfall series. Instead it's better to use tooltip to display values when those are equal to zero.

I disagree. This issue points out a bug in waterfall rendering - minPointLength is used to transform the actual data, not just "length" of individual columns. Of course you could just say minPointLength is simply not supported for waterfall, but then it should be clearly stated in the documentation.

I don't see though, how the functionality I propose above is much different from how minPointLength is used in other types of series. And even if it instead behaved exactly like for the column range series (the midpoint position of each column would be calculated and then it'd grow in both directions), how is the result any more misleading in the waterfall than in the column range?

@znevrly

This comment has been minimized.

Show comment
Hide comment
@znevrly

znevrly Jan 31, 2017

Hi,
please HighChart team, can you make a statement to this issues?

Thank you very much.

znevrly commented Jan 31, 2017

Hi,
please HighChart team, can you make a statement to this issues?

Thank you very much.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Feb 2, 2017

Contributor

@hon2a and @znevrly - my apologies for a delay. Of course, we will fix this. When fixed, minPointLength will not affect next points on an axis, only the current one.

Contributor

pawelfus commented Feb 2, 2017

@hon2a and @znevrly - my apologies for a delay. Of course, we will fix this. When fixed, minPointLength will not affect next points on an axis, only the current one.

@hon2a

This comment has been minimized.

Show comment
Hide comment
@hon2a

hon2a Feb 2, 2017

Great, thanks.

hon2a commented Feb 2, 2017

Great, thanks.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Feb 3, 2017

Contributor

Demo with fix applied: http://jsfiddle.net/1qu32g9n/10/ - let me know if you find any issues with that solution. Thank you!

Contributor

pawelfus commented Feb 3, 2017

Demo with fix applied: http://jsfiddle.net/1qu32g9n/10/ - let me know if you find any issues with that solution. Thank you!

@hon2a

This comment has been minimized.

Show comment
Hide comment
@hon2a

hon2a Feb 3, 2017

That looks awesome, can't wait for the release. Thank you!

hon2a commented Feb 3, 2017

That looks awesome, can't wait for the release. Thank you!

@rudovjan

This comment has been minimized.

Show comment
Hide comment
@rudovjan

rudovjan Apr 20, 2017

I'm afraid this is back with 5.0.10

rudovjan commented Apr 20, 2017

I'm afraid this is back with 5.0.10

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Apr 20, 2017

Contributor

Could you create a demo? This one looks fine: http://jsfiddle.net/1qu32g9n/11/

Contributor

pawelfus commented Apr 20, 2017

Could you create a demo? This one looks fine: http://jsfiddle.net/1qu32g9n/11/

@hon2a

This comment has been minimized.

Show comment
Hide comment
@hon2a

hon2a Apr 20, 2017

I'm afraid this is back with 5.0.10

It's not. This time it's a bug in our own plugin.

hon2a commented Apr 20, 2017

I'm afraid this is back with 5.0.10

It's not. This time it's a bug in our own plugin.

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