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.

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

Histogram bellcurve misaligned with standard deviation X-Axis with changes to chart element's width #7604

Closed
erichiggins opened this Issue Jan 2, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@erichiggins
Copy link

erichiggins commented Jan 2, 2018

Expected behaviour

The distribution bell curve should maintain the same alignment with the X-Axis, regardless of element width.

Actual behaviour

The distribution bell curve is not always aligned with the mean/average on the X-Axis as the width of the containing element changes.

Live demo with steps to reproduce

https://jsfiddle.net/e0yz8q4o/
Load the JS Fiddle above and slide the width of the output container.
Note the alignment of the top of the bell curve with the top X-Axis (which shows difference from the standard deviation).

This is pretty close to correct alignment.

screen shot 2018-01-02 at 2 14 45 pm

Nope.

screen shot 2018-01-02 at 2 14 54 pm

Wat?

screen shot 2018-01-02 at 2 15 12 pm

Affected browser(s)

Chrome (OSX)

@TorsteinHonsi

This comment has been minimized.

Copy link
Collaborator

TorsteinHonsi commented Jan 3, 2018

The X axes have aligned ticks by default, which makes them go to great length to keep the same number of ticks. This is prevented by setting alignTicks to false on the X axis level: https://jsfiddle.net/highcharts/e0yz8q4o/1/

@morganfree

  • Should we make our samples have alignTicks false to prevent this?
  • The last screenshot above must be a bug?
@erichiggins

This comment has been minimized.

Copy link
Author

erichiggins commented Jan 3, 2018

Thanks for the additional info, @TorsteinHonsi !
I took a look at the updated JSFiddle you provided with alignTicks: false, but it doesn't seem to actually resolve the issue. In a normal distribution curve, I'd expect the middle of the bell curve to be centered on the mean (average), and to be aligned with zero on the axis which displays standard deviations from the mean.

I believe this is demonstrated well in the interval and points in interval example, where μ is the mean and σ is the standard deviation

@morganfree

This comment has been minimized.

Copy link
Contributor

morganfree commented Jan 3, 2018

@erichiggins

In the demo both axes are hidden but, if they appear, the ticks from the top axis are not aligned as you would want. The bellcurve series is aware only about one axis and it is not concerned about computing/aligning other axes - use cases can differ so I believe that configuring other axes should be left to users.

In your case, if I understand it correctly, you can build third axis connected to the bellcurve axis with the ticks in specific positions and with the specific labels for the ticks.

let labels = {}

window.chart = Highcharts.chart('container', {
chart: {
  events: {
  // it needs to be done on load event - the information from the computed series are needed
    load () { 
       const {
         data,
         mean,
         standardDeviation,
         options: { pointsInInterval }
       } = this.get('bellcurve')

      // make the ticks - each ticks represents the unit of the standard deviation
       const tickPositions = data
         .filter((point, i) => !(i % pointsInInterval))
         .map(point => point.x)
         
       // make the dictionary for the tick labels, it will be used in the labels formatter
       labels = tickPositions.reduce((labels, tick) => {
         const value = (Math.abs(tick - mean) / standardDeviation).toFixed()
         
         labels[tick] = value
         // labels[tick] =  !Number(value) ? 'μ' : `${value}σ`
         
         return labels
       }, {})
       
       // update tickPositions
       this.xAxis[2].update({ tickPositions })
    }
  }
},
xAxis: [{
    title: { text: 'Data' },
    opposite: true,
    alignTicks: false,
    visible: false
}, {
    title: { text: 'Bell curve' },
    alignTicks: false
}, {
 // define third axis
  linkedTo: 1,
  alignTicks: false,
  opposite: true,
  labels: {
    // make use of the labels dictionary
    formatter () {
      return labels[this.value] || this.value
    }
  }
}],

live example: https://jsfiddle.net/dqfp48q6/

@TorsteinHonsi

Yes, we can set alignTicks to false to prevent it (axes are hidden anyway) but It seems to be a regression bug, 6.0.3 -> 6.0.4

6.0.3: https://jsfiddle.net/30eyoxx8/
6.0.4: https://jsfiddle.net/30eyoxx8/1/

@TorsteinHonsi

This comment has been minimized.

Copy link
Collaborator

TorsteinHonsi commented Jan 4, 2018

It fails since 4b0f2e3.

Yes, we can set alignTicks to false to prevent it (axes are hidden anyway) but It seems to be a regression bug, 6.0.3 -> 6.0.4

Do you agree that we set alignTicks to false in our demos, or is that not recommended? By setting it to false in the demos, we still leave it to the users, but we show that there's an option.

@morganfree

This comment has been minimized.

Copy link
Contributor

morganfree commented Jan 4, 2018

I agree - we can set alignTicks to false.

@KacperMadej

This comment has been minimized.

Copy link
Contributor

KacperMadej commented Jan 8, 2018

This looks like a more general issue caused by the mentioned commit - demo: https://jsfiddle.net/BlackLabel/9exu6941/1/

@KacperMadej KacperMadej added Bug and removed Undecided labels Jan 8, 2018

@KacperMadej KacperMadej self-assigned this Jan 9, 2018

@KacperMadej

This comment has been minimized.

Copy link
Contributor

KacperMadej commented Jan 23, 2018

After further testing turns out that for alignTicks to work correctly axes should have startOnTick and endOnTick set to true and xAxis by default has both set to false causing the problem more visible after the commit.

The startOnTick and endOnTick set to false for yAxis will prevent axes to be aligned properly.
Demo for before the commit - version 6.0.3 (not aligned): http://jsfiddle.net/BlackLabel/w7mfjy7h/
Demo for after the commit - version 6.0.4 (series plot outside of plot area): http://jsfiddle.net/BlackLabel/w7mfjy7h/1/
6.0.4 with startOnTick and endOnTick set to true (all ok): http://jsfiddle.net/BlackLabel/w7mfjy7h/2/

tl;dr
@TorsteinHonsi
This bug could be resolved by (1) adding info to the docs about startOnTick and endOnTick being required to be set to true and optionally by (2) forcing it at options level in case alignTicks is set to true. Also, (3) alignTicks is not documented on axis options level in API reference.

How to handle this?

  • 1
  • 2
  • 3

Of course I'm happy to handle this any other way you choose.

@TorsteinHonsi

This comment has been minimized.

Copy link
Collaborator

TorsteinHonsi commented Jan 26, 2018

What about another approach, simply disable alignTicks when startOnTick or endOnTick are false? I think that would be least surprising/confusing to the implementer. Plus it needs to be mentioned in the docs for alignTicks. As for implementation, it would be easy to check these options from within the tick extension logic.

Plus this will probably be backwards compatible with v6.0.3.

What do you think?

@KacperMadej

This comment has been minimized.

Copy link
Contributor

KacperMadej commented Jan 26, 2018

Looks good for me. I can handle this if you want.

What do you think @morganfree , @erichiggins ?

@erichiggins

This comment has been minimized.

Copy link
Author

erichiggins commented Jan 26, 2018

Sounds good to me. Thanks for the quick and thoughtful discussion on this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.