Skip to content

Commit

Permalink
Root Cause Fix: Prevent label generation from stopping one label too
Browse files Browse the repository at this point in the history
early due to cumulative precision errors in JS float representations.
This is also the root cause of issue apexcharts#430, allowing the
removal of the previous fix.

Root Cause Fix: getMinYMaxY():
1) not determining chart min and max according to series type, thus
returning incorrect values.
For example, e2e test "candlestick/candlestick-line.html" has
chart.type "line" while the two series' have types "line" and
"candlestick" respectively. The candlestick data min/max was being
determined as for a "line" series.
2) Erroneous if conditional logic allowing entry to all chart types
rather than just the targeted types.

Exclude line and area charts from +/-5% range expansion that is applied
to accommodate box and candlestick elements. See below: root cause fix.

Remove the unnecessary range expansion. Root cause fixed.

Several locations: remove code from previous fixes for the following
issues, as the examples provided in those issue reports display
correctly now:

Previous fix for issue apexcharts#492 is obsolete, possibly following
commit #4562468 that fixed cases where the Y axis missed including the
uppermost tick due to JS precision errors.

If the user defines options that are consistent amongst themselves,
apply them without adjustment.

In computing the provisional nice step size, set magMsd only to integer
subdivisions of 10, i.e. magPow*[1,2,5,10], eliminating unhelpful
values like magPow*[3,4,6,7,8,9]. This can be overridden by setting
stepSize and setting forceNiceScale: true. Then, if the given
stepSize is bigger than the range, or impractically small and
resulting in excessive ticks, apply it after normalizing it the chart
order of magnitude and checking again for consistency.
E.g. if the range = 0.4 and user sets stepSize: 8 (or 8000 or 0.0008),
then stepSize is obviously disproportionate and impractical.
Normalize it to 0.08 or 0.008 depending on tickAmount etc.

Rename the class in Scales.js from "Range" to "Scales" to avoid
being confused with the "Range" class defined in Range.js.

Modified unit tests:
Brought into line with changes introduced in the current branch
to prevent trivial failure reports.
New unit tests to verify priority of user defined options.

New e2e sample to verify duplicate label removal by formatter. AFAICS,
the only way to hide labels that don't correspond to tick values is
via the formatters. This is the cause of duplicate values on axes.

All working except for brush-charts, which don't call niceScale.

Add utility functions:
	getGCD() - return GCD of two numbers (not used in this version)
	getPrimeFactors() - return all prime factors of an integer
	mod() - a mod b: to overcome precision errors

Reduce ticks nicely as chart svg size gets smaller, if options allow.

Multi series/multi yaxis charts: attempt force consistent
tickAmounts so they line up with the grid. Per series user tickAmount
override this.

Adjust some tickAmount values in e2e samples now that user defined
tickAmount is honoured unless it conflicts with other user defined
options.

Set default ticks back to 10. With the proposed changes, the default
value only influences the computed nice step size and is reduced to
best fit the data after that.

The previous default of 5 has the following visual effect:
Most charts work with base 10 numbers, and 10/5=2, which tends to
produce tick spacings of 0.2, 2, 20, etc. This tends to visually
squeeze graphs toward the centre of the chart, leaving more of the
peripheral chart area unused, lowering resolution. Having 10 as the
default reverses this tendency, bringing data points closer to the
extremities of the range.

defaults.js: remove the default tickAmount = 6 for polarArea charts.
Let niceScale determine it if not user defined.

radar-with-polygon-fill.xml: remove user defined tickAmount = 7. Let
niceScale() determine it, otherwise 7 ticks are what will be drawn,
which will produce a sub-optimal chart for the sample data.

multiple-yaxes.xml: associate y axes with their correct series'.
The visual change in the chart is that the Income yaxis is scaled
correctly. Further highlights the other changes in this commit, showing
the alignment of all y scale labels with the grid.

Add NetBeans IDE config and ignore private resources.
  • Loading branch information
rocso committed Feb 5, 2024
1 parent 19d06cc commit ed2c666
Show file tree
Hide file tree
Showing 127 changed files with 452 additions and 189 deletions.
4 changes: 2 additions & 2 deletions samples/playground/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -4227,7 +4227,7 @@ export const optionsPattern = {
default: 10
},
tickAmount: {
title: 'Number of tick intervals to show',
title: 'Number of tick intervals to show. Will be honoured unless it conflicts with your other settings.',
type: Number,
default: 6
},
Expand All @@ -4243,7 +4243,7 @@ export const optionsPattern = {
},
forceNiceScale: {
title:
'If set to `true`, the y-axis scales are forced to generate nice looking rounded numbers even when min/max are provided. Turn this off if you manually set min/max and want it to be unchanged.',
'If set to `true`, y-axis scaling will try to make inferences from your option settings when conflicts would otherwise cause one or more to be ignored.',
type: Boolean,
default: false
},
Expand Down
2 changes: 1 addition & 1 deletion samples/react/area/area-with-negative.html
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@
}
},
yaxis: {
tickAmount: 3,
tickAmount: 4,
floating: false,

labels: {
Expand Down
3 changes: 2 additions & 1 deletion samples/react/mixed/multiple-yaxes.html
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
},
yaxis: [
{
seriesName: 'Income',
axisTicks: {
show: true,
},
Expand All @@ -137,7 +138,7 @@
}
},
{
seriesName: 'Income',
seriesName: 'Cashflow',
opposite: true,
axisTicks: {
show: true,
Expand Down
2 changes: 1 addition & 1 deletion samples/source/area/area-with-negative.xml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ xaxis: {
}
},
yaxis: {
tickAmount: 3,
tickAmount: 4,
floating: false,

labels: {
Expand Down
3 changes: 2 additions & 1 deletion samples/source/mixed/multiple-yaxes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ xaxis: {
},
yaxis: [
{
seriesName: 'Income',
axisTicks: {
show: true,
},
Expand All @@ -61,7 +62,7 @@ yaxis: [
}
},
{
seriesName: 'Income',
seriesName: 'Cashflow',
opposite: true,
axisTicks: {
show: true,
Expand Down
2 changes: 1 addition & 1 deletion samples/vanilla-js/area/area-with-negative.html
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@
}
},
yaxis: {
tickAmount: 3,
tickAmount: 4,
floating: false,

labels: {
Expand Down
3 changes: 2 additions & 1 deletion samples/vanilla-js/mixed/multiple-yaxes.html
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@
},
yaxis: [
{
seriesName: 'Income',
axisTicks: {
show: true,
},
Expand All @@ -120,7 +121,7 @@
}
},
{
seriesName: 'Income',
seriesName: 'Cashflow',
opposite: true,
axisTicks: {
show: true,
Expand Down
2 changes: 1 addition & 1 deletion samples/vue/area/area-with-negative.html
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@
}
},
yaxis: {
tickAmount: 3,
tickAmount: 4,
floating: false,

labels: {
Expand Down
3 changes: 2 additions & 1 deletion samples/vue/mixed/multiple-yaxes.html
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
},
yaxis: [
{
seriesName: 'Income',
axisTicks: {
show: true,
},
Expand All @@ -140,7 +141,7 @@
}
},
{
seriesName: 'Income',
seriesName: 'Cashflow',
opposite: true,
axisTicks: {
show: true,
Expand Down
60 changes: 28 additions & 32 deletions src/modules/Range.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,41 +80,37 @@ class Range {
highestY = Math.max(highestY, seriesMin[i][j])
}

if (
cnf.series[i].type === 'candlestick' ||
cnf.series[i].type === 'boxPlot' ||
cnf.series[i].type !== 'rangeArea' ||
cnf.series[i].type !== 'rangeBar'
) {
if (
cnf.series[i].type === 'candlestick' ||
cnf.series[i].type === 'boxPlot'
) {
// These series arrays are dual purpose:
// Array : CandleO, CandleH, CandleM, CandleL, CandleC
// Candlestick: O H L C
// Boxplot : Min Q1 Median Q3 Max
switch (cnf.series[i].type) {
case 'candlestick': {
if (typeof gl.seriesCandleC[i][j] !== 'undefined') {
maxY = Math.max(maxY, gl.seriesCandleO[i][j])
maxY = Math.max(maxY, gl.seriesCandleH[i][j])
maxY = Math.max(maxY, gl.seriesCandleL[i][j])
maxY = Math.max(maxY, gl.seriesCandleC[i][j])
if (this.w.config.chart.type === 'boxPlot') {
maxY = Math.max(maxY, gl.seriesCandleM[i][j])
}
lowestY = Math.min(lowestY, gl.seriesCandleL[i][j])
}
}

// there is a combo chart and the specified series in not either candlestick, boxplot, or rangeArea/rangeBar; find the max there
if (
cnf.series[i].type &&
(cnf.series[i].type !== 'candlestick' ||
cnf.series[i].type !== 'boxPlot' ||
cnf.series[i].type !== 'rangeArea' ||
cnf.series[i].type !== 'rangeBar')
) {
maxY = Math.max(maxY, gl.series[i][j])
lowestY = Math.min(lowestY, gl.series[i][j])
case 'boxPlot': {
if (typeof gl.seriesCandleC[i][j] !== 'undefined') {
maxY = Math.max(maxY, gl.seriesCandleC[i][j])
lowestY = Math.min(lowestY, gl.seriesCandleO[i][j])
}
}
}

highestY = maxY
// there is a combo chart and the specified series in not either candlestick, boxplot, or rangeArea/rangeBar; find the max there
if (
cnf.series[i].type &&
(cnf.series[i].type !== 'candlestick' &&
cnf.series[i].type !== 'boxPlot' &&
cnf.series[i].type !== 'rangeArea' &&
cnf.series[i].type !== 'rangeBar')
) {
maxY = Math.max(maxY, gl.series[i][j])
lowestY = Math.min(lowestY, gl.series[i][j])
}
highestY = maxY

if (
gl.seriesGoals[i] &&
Expand Down Expand Up @@ -492,7 +488,7 @@ class Range {
// fix #811
sX.push(
gl.seriesX[gl.maxValsInArrayIndex][
gl.seriesX[gl.maxValsInArrayIndex].length - 1
gl.seriesX[gl.maxValsInArrayIndex].length - 1
]
)
}
Expand Down Expand Up @@ -555,21 +551,21 @@ class Range {
if (gl.series[i][j] !== null && Utils.isNumber(gl.series[i][j])) {
gl.series[i][j] > 0
? (stackedPoss[group][j] +=
parseFloat(gl.series[i][j]) + 0.0001)
parseFloat(gl.series[i][j]) + 0.0001)
: (stackedNegs[group][j] += parseFloat(gl.series[i][j]))
}
}
}
})
})

Object.entries(stackedPoss).forEach(([key]) => {
Object.entries(stackedPoss).forEach(([key]) => {
stackedPoss[key].forEach((_, stgi) => {
gl.maxY = Math.max(gl.maxY, stackedPoss[key][stgi])
gl.minY = Math.min(gl.minY, stackedNegs[key][stgi])
})
})
}
}
}

export default Range
Loading

0 comments on commit ed2c666

Please sign in to comment.