Merge pull request #6340 from metabase/fix-failing-test

Fix timeseries tick calculations

frontend/src/metabase/visualizations/lib/apply_axis.js

@@ -66,12 +66,9 @@ function adjustXAxisTicksIfNeeded(axis, chartWidthPixels, xValues) {
     const tickAverageStringLength = averageStringLengthOfValues(xValues);
     const tickAverageWidthPixels  = tickAverageStringLength * APPROXIMATE_AVERAGE_CHAR_WIDTH_PIXELS;
 
-    console.log("tickAverageWidthPixels:", tickAverageWidthPixels); // NOCOMMIT
-
     // now figure out the approximate number of ticks we'll be able to show based on the width of the chart. Round
     // down so we error on the side of more space rather than less.
     const maxTicks = Math.floor(chartWidthPixels / tickAverageWidthPixels);
-    console.log("maxTicks:", maxTicks); // NOCOMMIT
 
     // finally, if the chart is currently showing more ticks than we think it can show, adjust it down
     if (getNumTicks(axis) > maxTicks) axis.ticks(maxTicks);
@@ -122,7 +119,7 @@ export function applyChartTimeseriesXAxis(chart, settings, series, { xValues, xD
 
         // Compute a sane interval to display based on the data granularity, domain, and chart width
         tickInterval = computeTimeseriesTicksInterval(xDomain, tickInterval, chart.width());
-        chart.xAxis().ticks(d3.time[tickInterval.interval], tickInterval.count);
+        chart.xAxis().ticks(tickInterval.rangeFn, tickInterval.count);
     } else {
         chart.xAxis().ticks(0);
     }

frontend/src/metabase/visualizations/lib/timeseries.js

 /* @flow weak */
 
+import d3 from "d3";
 import moment from "moment";
 import _ from "underscore";
 
@@ -29,29 +30,37 @@ export function dimensionIsTimeseries({ cols, rows }, i = 0) {
 // https://github.com/mbostock/d3/wiki/Time-Intervals
 // Use UTC methods to avoid issues with daylight savings
 // NOTE: smaller modulos within an interval type must be multiples of larger ones (e.x. can't do both 2 days and 7 days i.e. week)
+//
+// Count and time interval for axis.ticks() (see https://github.com/d3/d3-3.x-api-reference/blob/master/SVG-Axes.md#ticks)
+// is specified by rangeFn and count, e.g.
+//
+// xAxis.ticks(interval.rangeFn, interval.count) -> xAxis.ticks(d3.time.minutes, 15) // every 15 minutes
+//
+// TODO - I'm not sure what the appropriate thing to put for rangeFn for milliseconds is. This matches the previous
+// behavior, which may have been wrong in the first place. See https://github.com/d3/d3/issues/1529 for a similar issue
 const TIMESERIES_INTERVALS = [
-    { interval: "ms",     count: 1,   testFn: (d) => 0                            }, //  (0) millisecond
-    { interval: "second", count: 1,   testFn: (d) => parseTimestamp(d).milliseconds() }, //  (1) 1 second
-    { interval: "second", count: 5,   testFn: (d) => parseTimestamp(d).seconds() % 5  }, //  (2) 5 seconds
-    { interval: "second", count: 15,  testFn: (d) => parseTimestamp(d).seconds() % 15 }, //  (3) 15 seconds
-    { interval: "second", count: 30,  testFn: (d) => parseTimestamp(d).seconds() % 30 }, //  (4) 30 seconds
-    { interval: "minute", count: 1,   testFn: (d) => parseTimestamp(d).seconds()      }, //  (5) 1 minute
-    { interval: "minute", count: 5,   testFn: (d) => parseTimestamp(d).minutes() % 5  }, //  (6) 5 minutes
-    { interval: "minute", count: 15,  testFn: (d) => parseTimestamp(d).minutes() % 15 }, //  (7) 15 minutes
-    { interval: "minute", count: 30,  testFn: (d) => parseTimestamp(d).minutes() % 30 }, //  (8) 30 minutes
-    { interval: "hour",   count: 1,   testFn: (d) => parseTimestamp(d).minutes()      }, //  (9) 1 hour
-    { interval: "hour",   count: 3,   testFn: (d) => parseTimestamp(d).hours() % 3    }, // (10) 3 hours
-    { interval: "hour",   count: 6,   testFn: (d) => parseTimestamp(d).hours() % 6    }, // (11) 6 hours
-    { interval: "hour",   count: 12,  testFn: (d) => parseTimestamp(d).hours() % 12   }, // (12) 12 hours
-    { interval: "day",    count: 1,   testFn: (d) => parseTimestamp(d).hours()        }, // (13) 1 day
-    { interval: "week",   count: 1,   testFn: (d) => parseTimestamp(d).date() % 7     }, // (14) 7 days / 1 week
-    { interval: "month",  count: 1,   testFn: (d) => parseTimestamp(d).date()         }, // (15) 1 months
-    { interval: "month",  count: 3,   testFn: (d) => parseTimestamp(d).month() % 3    }, // (16) 3 months / 1 quarter
-    { interval: "year",   count: 1,   testFn: (d) => parseTimestamp(d).month()        }, // (17) 1 year
-    { interval: "year",   count: 5,   testFn: (d) => parseTimestamp(d).year() % 5     }, // (18) 5 year
-    { interval: "year",   count: 10,  testFn: (d) => parseTimestamp(d).year() % 10    }, // (19) 10 year
-    { interval: "year",   count: 50,  testFn: (d) => parseTimestamp(d).year() % 50    }, // (20) 50 year
-    { interval: "year",   count: 100, testFn: (d) => parseTimestamp(d).year() % 100   }  // (21) 100 year
+    { interval: "ms",     count: 1,   rangeFn: undefined,       testFn: (d) => 0                            }, //  (0) millisecond
+    { interval: "second", count: 1,   rangeFn: d3.time.seconds, testFn: (d) => parseTimestamp(d).milliseconds() }, //  (1) 1 second
+    { interval: "second", count: 5,   rangeFn: d3.time.seconds, testFn: (d) => parseTimestamp(d).seconds() % 5  }, //  (2) 5 seconds
+    { interval: "second", count: 15,  rangeFn: d3.time.seconds, testFn: (d) => parseTimestamp(d).seconds() % 15 }, //  (3) 15 seconds
+    { interval: "second", count: 30,  rangeFn: d3.time.seconds, testFn: (d) => parseTimestamp(d).seconds() % 30 }, //  (4) 30 seconds
+    { interval: "minute", count: 1,   rangeFn: d3.time.minutes, testFn: (d) => parseTimestamp(d).seconds()      }, //  (5) 1 minute
+    { interval: "minute", count: 5,   rangeFn: d3.time.minutes, testFn: (d) => parseTimestamp(d).minutes() % 5  }, //  (6) 5 minutes
+    { interval: "minute", count: 15,  rangeFn: d3.time.minutes, testFn: (d) => parseTimestamp(d).minutes() % 15 }, //  (7) 15 minutes
+    { interval: "minute", count: 30,  rangeFn: d3.time.minutes, testFn: (d) => parseTimestamp(d).minutes() % 30 }, //  (8) 30 minutes
+    { interval: "hour",   count: 1,   rangeFn: d3.time.hours,   testFn: (d) => parseTimestamp(d).minutes()      }, //  (9) 1 hour
+    { interval: "hour",   count: 3,   rangeFn: d3.time.hours,   testFn: (d) => parseTimestamp(d).hours() % 3    }, // (10) 3 hours
+    { interval: "hour",   count: 6,   rangeFn: d3.time.hours,   testFn: (d) => parseTimestamp(d).hours() % 6    }, // (11) 6 hours
+    { interval: "hour",   count: 12,  rangeFn: d3.time.hours,   testFn: (d) => parseTimestamp(d).hours() % 12   }, // (12) 12 hours
+    { interval: "day",    count: 1,   rangeFn: d3.time.days,    testFn: (d) => parseTimestamp(d).hours()        }, // (13) 1 day
+    { interval: "week",   count: 1,   rangeFn: d3.time.weeks,   testFn: (d) => parseTimestamp(d).date() % 7     }, // (14) 7 days / 1 week
+    { interval: "month",  count: 1,   rangeFn: d3.time.months,  testFn: (d) => parseTimestamp(d).date()         }, // (15) 1 months
+    { interval: "month",  count: 3,   rangeFn: d3.time.months,  testFn: (d) => parseTimestamp(d).month() % 3    }, // (16) 3 months / 1 quarter
+    { interval: "year",   count: 1,   rangeFn: d3.time.years,   testFn: (d) => parseTimestamp(d).month()        }, // (17) 1 year
+    { interval: "year",   count: 5,   rangeFn: d3.time.years,   testFn: (d) => parseTimestamp(d).year() % 5     }, // (18) 5 year
+    { interval: "year",   count: 10,  rangeFn: d3.time.years,   testFn: (d) => parseTimestamp(d).year() % 10    }, // (19) 10 year
+    { interval: "year",   count: 50,  rangeFn: d3.time.years,   testFn: (d) => parseTimestamp(d).year() % 50    }, // (20) 50 year
+    { interval: "year",   count: 100, rangeFn: d3.time.years,   testFn: (d) => parseTimestamp(d).year() % 100   }  // (21) 100 year
 ];
 
 // mapping from Metabase "unit" to d3 intervals above
@@ -82,8 +91,8 @@ function computeTimeseriesDataInvervalIndex(xValues, unit) {
     for (let xValue of xValues) {
         // Only need to check more granular than the current interval
         for (let i = 0; i < TIMESERIES_INTERVALS.length && i < index; i++) {
-            let interval = TIMESERIES_INTERVALS[i];
-            let value = interval.testFn(xValue);
+            const interval = TIMESERIES_INTERVALS[i];
+            const value    = interval.testFn(xValue);
             if (values[i] === undefined) {
                 values[i] = value;
             } else if (values[i] !== value) {
@@ -98,22 +107,66 @@ export function computeTimeseriesDataInverval(xValues, unit) {
     return TIMESERIES_INTERVALS[computeTimeseriesDataInvervalIndex(xValues, unit)];
 }
 
-export function computeTimeseriesTicksInterval(xDomain, xInterval, chartWidth) {
-    /// number of pixels each tick should get. TODO - this doesn't take into account the width of each tick, leading to overlappery
-    const MIN_PIXELS_PER_TICK = 100;
-
-    // If the interval that matches the data granularity results in too many ticks reduce the granularity until it doesn't.
-    // TODO: compute this directly instead of iteratively
-    const maxTickCount = Math.round(chartWidth / MIN_PIXELS_PER_TICK);
-
-    let index = _.findIndex(TIMESERIES_INTERVALS, ({ interval, count }) => interval === xInterval.interval && count === xInterval.count);
-    for (; index < TIMESERIES_INTERVALS.length - 1; index++) {
-        const interval   = TIMESERIES_INTERVALS[index];
-        const intervalMs = moment(0).add(interval.count, interval.interval).valueOf();
-        const tickCount  = (xDomain[1] - xDomain[0]) / intervalMs;
-        if (tickCount <= maxTickCount) {
-            break;
-        }
+// ------------------------- Computing the TIMESERIES_INTERVALS entry to use for a chart ------------------------- //
+
+/// The number of milliseconds between each tick for an entry in TIMESERIES_INTERVALS.
+/// For example a "5 seconds" interval would have a tick "distance" of 5000 milliseconds.
+function intervalTickDistanceMilliseconds(interval) {
+    // add COUNT nuumber of INTERVALS to the UNIX timestamp 0. e.g. add '5 hours' to 0. Then get the new timestamp
+    // (in milliseconds). Since we added to 0 this will be the interval between each tick
+    return moment(0).add(interval.count, interval.interval).valueOf();
+}
+
+/// Return the number of ticks we can expect to see over a time range using the TIMESERIES_INTERVALS entry interval.
+/// for example a "5 seconds" interval over a time range of a minute should have an expected tick count of 20.
+function expectedTickCount(interval, timeRangeMilliseconds) {
+    return Math.ceil(timeRangeMilliseconds / intervalTickDistanceMilliseconds(interval));
+}
+
+/// Get the appropriate tick interval option option from the TIMESERIES_INTERVALS above based on the xAxis bucketing
+/// and the max number of ticks we want to show (itself calculated from chart width).
+function timeseriesTicksInterval(xInterval, timeRangeMilliseconds, maxTickCount) {
+    // first we want to find out where in TIMESERIES_INTERVALS we should start looking for a good match. Find the
+    // interval with a matching interval and count (e.g. `hour` and `1`) and we'll start there.
+    let initialIndex = _.findIndex(TIMESERIES_INTERVALS, ({ interval, count }) => {
+        return interval === xInterval.interval && count === xInterval.count;
+    });
+    // if we weren't able to find soemthing matching then we'll start from the beginning and try everything
+    if (initialIndex === -1) initialIndex = 0;
+
+    // now starting at the TIMESERIES_INTERVALS entry in question, calculate the expected tick count for that interval
+    // based on the time range we are displaying. If the expected tick count is less than or equal to the target
+    // maxTickCount, we can go ahead and use this interval. Otherwise continue on to the next larger interval, for
+    // example every 3 hours instead of every one hour. Continue until we find something with an interval large enough
+    // to keep the total tick count under the max tick count
+    for (const interval of _.rest(TIMESERIES_INTERVALS, initialIndex)) {
+        if (expectedTickCount(interval, timeRangeMilliseconds) <= maxTickCount) return interval;
     }
-    return TIMESERIES_INTERVALS[index];
+
+    // If we still failed to find an interval that will produce less ticks than the max then fall back to the largest
+    // tick interval (every 100 years)
+    return TIMESERIES_INTERVALS[TIMESERIES_INTERVALS.length - 1];
+}
+
+/// return the maximum number of ticks to show for a timeseries chart of a given width. Unlike other chart types, this
+/// isn't smart enough to actually estimate how much space each tick will take. Instead the estimated with is
+/// hardcoded below.
+/// TODO - it would be nice to rework this a bit so we
+function maxTicksForChartWidth(chartWidth) {
+    const MIN_PIXELS_PER_TICK = 160;
+    return Math.floor(chartWidth / MIN_PIXELS_PER_TICK); // round down so we don't end up with too many ticks
+}
+
+/// return the range, in milliseconds, of the xDomain. ("Range" in this sense refers to the total "width"" of the
+/// chart in millisecodns.)
+function timeRangeMilliseconds(xDomain) {
+    const startTime = xDomain[0]; // these are UNIX timestamps in milliseconds
+    const endTime   = xDomain[1];
+    return endTime - startTime;
+}
+
+/// return the appropriate entry in TIMESERIES_INTERVALS for a given chart with domain, interval, and width.
+/// The entry is used to calculate how often a tick should be displayed for this chart (e.g. one tick every 5 minutes)
+export function computeTimeseriesTicksInterval(xDomain, xInterval, chartWidth) {
+    return timeseriesTicksInterval(xInterval, timeRangeMilliseconds(xDomain), maxTicksForChartWidth(chartWidth));
 }

frontend/src/metabase/visualizations/lib/utils.js

@@ -31,16 +31,16 @@ export function columnsAreValid(colNames, data, filter = () => true) {
 
 // computed size properties (drop 'px' and convert string -> Number)
 function getComputedSizeProperty(prop, element) {
-    var val = document.defaultView.getComputedStyle(element, null).getPropertyValue(prop);
-    return val ? parseFloat(val.replace("px", "")) : null;
+    const val = document.defaultView.getComputedStyle(element, null).getPropertyValue(prop);
+    return val ? parseFloat(val.replace("px", "")) : 0;
 }
 
 /// height available for rendering the card
 export function getAvailableCanvasHeight(element) {
-    var parent = element.parentElement,
-        parentHeight = getComputedSizeProperty("height", parent),
-        parentPaddingTop = getComputedSizeProperty("padding-top", parent),
-        parentPaddingBottom = getComputedSizeProperty("padding-bottom", parent);
+    const parent              = element.parentElement;
+    const parentHeight        = getComputedSizeProperty("height", parent);
+    const parentPaddingTop    = getComputedSizeProperty("padding-top", parent);
+    const parentPaddingBottom = getComputedSizeProperty("padding-bottom", parent);
 
     // NOTE: if this magic number is not 3 we can get into infinite re-render loops
     return parentHeight - parentPaddingTop - parentPaddingBottom - 3; // why the magic number :/
@@ -48,10 +48,10 @@ export function getAvailableCanvasHeight(element) {
 
 /// width available for rendering the card
 export function getAvailableCanvasWidth(element) {
-    var parent = element.parentElement,
-        parentWidth = getComputedSizeProperty("width", parent),
-        parentPaddingLeft = getComputedSizeProperty("padding-left", parent),
-        parentPaddingRight = getComputedSizeProperty("padding-right", parent);
+    const parent             = element.parentElement;
+    const parentWidth        = getComputedSizeProperty("width", parent);
+    const parentPaddingLeft  = getComputedSizeProperty("padding-left", parent);
+    const parentPaddingRight = getComputedSizeProperty("padding-right", parent);
 
     return parentWidth - parentPaddingLeft - parentPaddingRight;
 }

frontend/test/visualizations/components/LineAreaBarRenderer.unit.spec.js

@@ -16,12 +16,12 @@ describe("LineAreaBarRenderer", () => {
     let element;
 
     beforeEach(function() {
-        document.body.insertAdjacentHTML('afterbegin', '<div id="fixture" style="height: 800px; width: 1200px;">');
+        document.body.insertAdjacentHTML('afterbegin', '<div id="fixture-parent" style="height: 800px; width: 1200px;"><div id="fixture" /></div>');
         element = document.getElementById('fixture');
     });
 
     afterEach(function() {
-        document.body.removeChild(document.getElementById('fixture'));
+        document.body.removeChild(document.getElementById('fixture-parent'));
     });
 
     it("should display numeric year in X-axis and tooltip correctly", () => {
@@ -61,43 +61,44 @@ describe("LineAreaBarRenderer", () => {
 
     ["Z", ...ALL_TZS].forEach(tz =>
         it("should display hourly data (in " + tz + " timezone) in X axis and tooltip consistently", () => {
-        return new Promise((resolve, reject) => {
-            let rows = [
-                ["2016-10-03T20:00:00.000" + tz, 1],
-                ["2016-10-03T21:00:00.000" + tz, 1],
-            ];
-
-            renderTimeseriesLine({
-                rowsOfSeries: [rows],
-                unit: "hour",
-                onHoverChange: (hover) => {
-                    try {
-                        let expected = rows.map(row => formatValue(row[0], {column: DateTimeColumn({unit: "hour"})}));
-                        expect(formatValue(hover.data[0].value, {column: hover.data[0].col})).toEqual(
-                            expected[0]
-                        );
-                        expect(qsa(".axis.x .tick text").map(e => e.textContent)).toEqual(expected);
-                        resolve();
-                    } catch(e) {
-                        reject(e)
+            return new Promise((resolve, reject) => {
+                const rows = [
+                    ["2016-10-03T20:00:00.000" + tz, 1],
+                    ["2016-10-03T21:00:00.000" + tz, 1],
+                ];
+
+                renderTimeseriesLine({
+                    rowsOfSeries: [rows],
+                    unit: "hour",
+                    onHoverChange: (hover) => {
+                        try {
+                            let expected = rows.map(row => formatValue(row[0], {column: DateTimeColumn({unit: "hour"})}));
+                            expect(formatValue(hover.data[0].value, {column: hover.data[0].col})).toEqual(
+                                expected[0]
+                            );
+                            expect(qsa(".axis.x .tick text").map(e => e.textContent)).toEqual(expected);
+                            resolve();
+                        } catch(e) {
+                            reject(e)
+                        }
                     }
-                }
-            })
+                });
 
-            dispatchUIEvent(qs(".dot"), "mousemove");
+                dispatchUIEvent(qs(".dot"), "mousemove");
+            });
         })
-    })
-)
+    );
 
     it("should display hourly data (in the browser's timezone) in X axis and tooltip consistently and correctly", () => {
         return new Promise((resolve, reject) => {
-            let tz = BROWSER_TZ;
-            let rows = [
+            const tz = BROWSER_TZ;
+            const rows = [
                 ["2016-01-01T01:00:00.000" + tz, 1],
                 ["2016-01-01T02:00:00.000" + tz, 1],
                 ["2016-01-01T03:00:00.000" + tz, 1],
                 ["2016-01-01T04:00:00.000" + tz, 1]
             ];
+
             renderTimeseriesLine({
                 rowsOfSeries: [rows],
                 unit: "hour",
@@ -110,23 +111,22 @@ describe("LineAreaBarRenderer", () => {
                             '1 AM - January 1, 2016'
                         );
 
-                        // Doesn't return the correct ticks in Jest for some reason
                         expect(qsa(".axis.x .tick text").map(e => e.textContent)).toEqual([
                             '1 AM - January 1, 2016',
-                            // '2 AM - January 1, 2016',
-                            // '3 AM - January 1, 2016',
+                            '2 AM - January 1, 2016',
+                            '3 AM - January 1, 2016',
                             '4 AM - January 1, 2016'
                         ]);
 
                         resolve();
                     } catch (e) {
-                        reject(e)
+                        reject(e);
                     }
                 }
             });
 
             dispatchUIEvent(qs(".dot"), "mousemove");
-        })
+        });
     });
 
     describe("should render correctly a compound line graph", () => {
@@ -301,4 +301,3 @@ describe("LineAreaBarRenderer", () => {
         });
     }
 });
-