diff --git a/frontend/src/metabase/visualizations/lib/apply_axis.js b/frontend/src/metabase/visualizations/lib/apply_axis.js
index 745ea5ed637a5f891d36bc4ab11b26d2027936e3..006b4c58c4ffc86f043225ea375fbc19210a70ef 100644
--- a/frontend/src/metabase/visualizations/lib/apply_axis.js
+++ b/frontend/src/metabase/visualizations/lib/apply_axis.js
@@ -10,7 +10,7 @@ import { formatValue } from "metabase/lib/formatting";
import { computeTimeseriesTicksInterval } from "./timeseries";
import timeseriesScale from "./timeseriesScale";
-import { isMultipleOf, getModuloScaleFactor } from "./numeric";
+import { isMultipleOf } from "./numeric";
import { getFriendlyName } from "./utils";
import { isHistogram } from "./renderer_utils";
@@ -220,14 +220,9 @@ export function applyChartQuantitativeXAxis(
);
adjustXAxisTicksIfNeeded(chart.xAxis(), chart.width(), xValues);
- // if xInterval is less than 1 we need to scale the values before doing
- // modulo comparison. isMultipleOf will compute it for us but we can do it
- // once here as an optimization
- const modulorScale = getModuloScaleFactor(xInterval);
-
chart.xAxis().tickFormat(d => {
// don't show ticks that aren't multiples of xInterval
- if (isMultipleOf(d, xInterval, modulorScale)) {
+ if (isMultipleOf(d, xInterval)) {
return formatValue(d, {
...chart.settings.column(dimensionColumn),
type: "axis",
diff --git a/frontend/src/metabase/visualizations/lib/numeric.js b/frontend/src/metabase/visualizations/lib/numeric.js
index df1d6774b47d8d7e546c3574b6f326f7d028e753..8fff9fd582f2f5cb037b7e5722d52f1143455caf 100644
--- a/frontend/src/metabase/visualizations/lib/numeric.js
+++ b/frontend/src/metabase/visualizations/lib/numeric.js
@@ -6,6 +6,9 @@ export function dimensionIsNumeric({ cols, rows }, i = 0) {
return isNumeric(cols[i]) || typeof (rows[0] && rows[0][i]) === "number";
}
+// We seem to run into float bugs if we get any more precise than this.
+const SMALLEST_PRECISION_EXP = -14;
+
export function precision(a) {
if (!isFinite(a)) {
return 0;
@@ -13,14 +16,15 @@ export function precision(a) {
if (!a) {
return 0;
}
- let e = 1;
- while (Math.round(a / e) !== a / e) {
- e /= 10;
- }
- while (Math.round(a / Math.pow(10, e)) === a / Math.pow(10, e)) {
- e *= 10;
+
+ // Find the largest power of ten needed to evenly divide the value. We start
+ // with the power of ten greater than the value and walk backwards until we
+ // hit our limit of SMALLEST_PRECISION_EXP or isMultipleOf returns true.
+ let e = Math.ceil(Math.log10(Math.abs(a)));
+ while (e > SMALLEST_PRECISION_EXP && !isMultipleOf(a, Math.pow(10, e))) {
+ e--;
}
- return e;
+ return Math.pow(10, e);
}
export function decimalCount(a) {
@@ -59,8 +63,10 @@ export function logTickFormat(axis) {
axis.tickFormat(formatTick);
}
-export const getModuloScaleFactor = base =>
- Math.max(1, Math.pow(10, Math.ceil(Math.log10(1 / base))));
-
-export const isMultipleOf = (value, base, scale = getModuloScaleFactor(base)) =>
- (value * scale) % (base * scale) === 0;
+export const isMultipleOf = (value, base) => {
+ // Ideally we could use Number.EPSILON as constant diffThreshold here.
+ // However, we sometimes see very small errors that are bigger than EPSILON.
+ // For example, when called 1.23456789 and 1e-8 we see a diff of ~1e-16.
+ const diffThreshold = Math.pow(10, SMALLEST_PRECISION_EXP);
+ return Math.abs(value - Math.round(value / base) * base) < diffThreshold;
+};
diff --git a/frontend/test/metabase/visualizations/lib/numeric.unit.spec.js b/frontend/test/metabase/visualizations/lib/numeric.unit.spec.js
index 89cb3d0dc1c302893c5df6014521ff3cbf6c0454..2daea4a183b0a97ce63064fbc17aca229c26fc53 100644
--- a/frontend/test/metabase/visualizations/lib/numeric.unit.spec.js
+++ b/frontend/test/metabase/visualizations/lib/numeric.unit.spec.js
@@ -2,7 +2,6 @@ import {
precision,
computeNumericDataInverval,
isMultipleOf,
- getModuloScaleFactor,
} from "metabase/visualizations/lib/numeric";
describe("visualization.lib.numeric", () => {
@@ -23,10 +22,25 @@ describe("visualization.lib.numeric", () => {
[0.9, 0.1],
[-0.5, 0.1],
[-0.9, 0.1],
+ [1.23, 0.01],
+ [1.234, 1e-3],
+ [1.2345, 1e-4],
+ [1.23456, 1e-5],
+ [1.234567, 1e-6],
+ [1.2345678, 1e-7],
+ [1.23456789, 1e-8],
+ [-1.23456789, 1e-8],
+ [-1.2345678912345, 1e-13],
+ [-1.23456789123456, 1e-14],
+ // very precise numbers are cut off at 10^-14
+ [-1.23456789123456789123456789, 1e-14],
];
- for (const c of CASES) {
- it("precision of " + c[0] + " should be " + c[1], () => {
- expect(precision(c[0])).toEqual(c[1]);
+ for (const [n, p] of CASES) {
+ it(`precision of ${n} should be ${p}`, () => {
+ expect(Math.abs(precision(n) - p) < Number.EPSILON).toBe(true);
+ // The expect above doesn't print out the relevant values for failures.
+ // The next line fails but can be useful when debugging.
+ // expect(precision(n)).toBe(p);
});
}
});
@@ -46,20 +60,6 @@ describe("visualization.lib.numeric", () => {
});
}
});
- describe("getModuloScaleFactor", () => {
- [
- [0.01, 100],
- [0.05, 100],
- [0.1, 10],
- [1, 1],
- [2, 1],
- [10, 1],
- [10 ** 10, 1],
- ].map(([value, expected]) =>
- it(`should return ${expected} for ${value}`, () =>
- expect(getModuloScaleFactor(value)).toBe(expected)),
- );
- });
describe("isMultipleOf", () => {
[
[1, 0.1, true],
@@ -71,9 +71,18 @@ describe("visualization.lib.numeric", () => {
[0.25, 0.1, false],
[0.000000001, 0.0000000001, true],
[0.0000000001, 0.000000001, false],
+ [100, 1e-14, true],
].map(([value, base, expected]) =>
it(`${value} ${expected ? "is" : "is not"} a multiple of ${base}`, () =>
expect(isMultipleOf(value, base)).toBe(expected)),
);
+
+ // With the current implementation this is guaranteed to be true. This test
+ // is left in incase that implementation changes.
+ [123456.123456, -123456.123456, 1.23456789, -1.23456789].map(value =>
+ it(`${value} should be a multiple of its precision (${precision(
+ value,
+ )})`, () => expect(isMultipleOf(value, precision(value))).toBe(true)),
+ );
});
});