Fix broken x axis (#11824)

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",

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;
+};

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)),
+    );
   });
 });