diff --git a/front_end/src/components/charts/fan_chart.tsx b/front_end/src/components/charts/fan_chart.tsx index 633af04d31..7e37c7be05 100644 --- a/front_end/src/components/charts/fan_chart.tsx +++ b/front_end/src/components/charts/fan_chart.tsx @@ -48,6 +48,7 @@ import { getAxisLeftPadding, getAxisRightPadding, getTickLabelFontSize, + widenDomainToTicks, } from "@/utils/charts/axis"; import { calculateCharWidth, @@ -881,6 +882,11 @@ function buildChartData({ } }); + // Widen yDomain to encompass every tick — log-warped questions cluster + // their data in a small slice and the auto-zoomed yDomain would clip + // tick labels that fall outside it. + const yDomain = widenDomainToTicks(finalZoom, yScale.ticks); + return { communityLines, communityAreas, @@ -889,7 +895,7 @@ function buildChartData({ resolutionPoints, emptyPoints, yScale, - yDomain: finalZoom, + yDomain, }; } diff --git a/front_end/src/components/charts/group_chart.tsx b/front_end/src/components/charts/group_chart.tsx index 72c1f40d88..33a750fe22 100644 --- a/front_end/src/components/charts/group_chart.tsx +++ b/front_end/src/components/charts/group_chart.tsx @@ -44,6 +44,7 @@ import { generateTimestampXScale, getAxisRightPadding, getTickLabelFontSize, + widenDomainToTicks, } from "@/utils/charts/axis"; import { getResolutionPoint } from "@/utils/charts/resolution"; import { scaleInternalLocation, unscaleNominalLocation } from "@/utils/math"; @@ -1062,7 +1063,12 @@ function buildChartData({ alwaysShowTicks: true, }); - return { xScale, yScale, graphs, xDomain, yDomain: zoomedYDomain }; + // Widen yDomain to encompass every tick — log-warped questions cluster + // their data in a small slice and the auto-zoomed yDomain would clip + // tick labels that fall outside it. + const yDomain = widenDomainToTicks(zoomedYDomain, yScale.ticks); + + return { xScale, yScale, graphs, xDomain, yDomain }; } // Define a custom "X" symbol function diff --git a/front_end/src/utils/charts/__tests__/axis.test.ts b/front_end/src/utils/charts/__tests__/axis.test.ts index 55c036be7f..a0eed9dc6f 100644 --- a/front_end/src/utils/charts/__tests__/axis.test.ts +++ b/front_end/src/utils/charts/__tests__/axis.test.ts @@ -201,6 +201,46 @@ describe("generateScale", () => { .filter((label) => label !== ""); expect(formattedLabels.length).toBeGreaterThan(2); }); + + it("picks nice display labels on positive log axes (no awkward endpoints)", () => { + // Regression for the [1, 52.7] fan card: the previous algorithm + // preserved the range_max as a tick literally, producing labels + // like "52.7". Picking nice values in display space avoids that. + const params = { + displayType: QuestionType.Numeric, + axisLength: 200, + direction: ScaleDirection.Vertical, + domain: [0, 1] as [number, number], + zoomedDomain: [0, 1] as [number, number], + scaling: { + range_min: 1, + range_max: 52.7, + zero_point: 0, + }, + forceTickCount: 5, + }; + + const scale = generateScale(params); + const labels = scale.ticks + .map((t) => scale.tickFormat(t)) + .filter((s) => s !== ""); + + expect(labels.length).toBeGreaterThanOrEqual(2); + expect(labels).not.toContain("52.7"); + + // The labels should be evenly spaced (in display space) by a nice + // step in {1, 2, 5} * 10^k. + const values = labels.map(Number); + values.forEach((v) => expect(Number.isFinite(v)).toBe(true)); + const step = (values[1] as number) - (values[0] as number); + const log = Math.log10(Math.abs(step)); + const exponent = + Math.abs(log - Math.round(log)) < 1e-3 + ? Math.round(log) + : Math.floor(log); + const mantissa = Math.abs(step) / Math.pow(10, exponent); + expect([1, 2, 5]).toContain(Math.round(mantissa)); + }); }); describe("graph ticks formatting", () => { @@ -230,6 +270,86 @@ describe("generateScale", () => { }); }); + describe("d3.ticks linear scaling", () => { + it("produces nice ticks for an awkward range like [-27.7, 20]", () => { + // Given: a question with an ugly range_min that previously yielded + // endpoints like -27.7 and 52.7 with the evenly-spaced algorithm. + const params = { + displayType: QuestionType.Numeric, + axisLength: 200, + direction: ScaleDirection.Vertical, + domain: [-27.7, 20] as [number, number], + zoomedDomain: [-27.7, 20] as [number, number], + scaling: { + range_min: -27.7, + range_max: 20, + zero_point: null, + }, + }; + + // When + const scale = generateScale(params); + + // Then: labeled (major) ticks should be nice — multiples of a step + // that itself is a member of {1,2,5} * 10^k. + const labeledTicks = scale.ticks.filter( + (t) => scale.tickFormat(t) !== "" + ); + expect(labeledTicks.length).toBeGreaterThan(1); + + const first = labeledTicks[0] as number; + const second = labeledTicks[1] as number; + const last = labeledTicks[labeledTicks.length - 1] as number; + const step = second - first; + + const exponent = Math.floor(Math.log10(Math.abs(step))); + const mantissa = step / Math.pow(10, exponent); + expect([1, 2, 5]).toContain(Math.round(mantissa)); + + const eps = Math.abs(step) * 1e-9; + expect(Math.abs(first - Math.round(first / step) * step)).toBeLessThan( + eps + ); + expect(Math.abs(last - Math.round(last / step) * step)).toBeLessThan(eps); + }); + + it("produces nice ticks for [0, 100]", () => { + // Given + const params = { + displayType: QuestionType.Numeric, + axisLength: 200, + direction: ScaleDirection.Vertical, + domain: [0, 100] as [number, number], + zoomedDomain: [0, 100] as [number, number], + scaling: { + range_min: 0, + range_max: 100, + zero_point: null, + }, + }; + + // When + const scale = generateScale(params); + + // Then: labeled (major) ticks should match one of d3's expected outputs. + const labeledTicks = scale.ticks.filter( + (t) => scale.tickFormat(t) !== "" + ); + // With the global max-4 cap, d3.ticks(0, 100, 4) returns 6 ticks + // (step 20) which exceeds the cap; the helper falls back to c=3 → + // [0, 50, 100]. [0, 25, 50, 75, 100] and the 6-tick options aren't + // achievable under the cap. + const valid1 = [0, 50, 100]; + const valid2 = [0, 25, 50, 75, 100]; + const arraysEqual = (a: number[], b: number[]) => + a.length === b.length && a.every((v, i) => v === b[i]); + + expect( + arraysEqual(labeledTicks, valid1) || arraysEqual(labeledTicks, valid2) + ).toBe(true); + }); + }); + describe("graph force ticks count", () => { it("helper should return specified number of ticks", () => { // Given @@ -254,6 +374,139 @@ describe("generateScale", () => { // Then expect(scale.ticks.length).toBe(FORCE_TICK_COUNT); }); + + it("treats forceTickCount as a hint for numeric linear scales so labels stay nice", () => { + // Regression: previously forceTickCount on a numeric axis bypassed + // d3.ticks and produced evenly-spaced labels like + // [-27.7, -15.78, -3.85, 8.08, 20]. We now expect d3-nice values. + const params = { + displayType: QuestionType.Numeric, + axisLength: 400, + direction: ScaleDirection.Vertical, + domain: [0, 1] as [number, number], + zoomedDomain: [0, 1] as [number, number], + scaling: { + range_min: -27.7, + range_max: 20, + zero_point: null, + }, + forceTickCount: 5, + }; + + const scale = generateScale(params); + + const labeledRangeValues = scale.ticks + .filter((t) => scale.tickFormat(t) !== "") + .map((t) => -27.7 + t * (20 - -27.7)); + + expect(labeledRangeValues.length).toBeGreaterThan(1); + // Domain ticks are rounded to 6 decimals before being mapped back to + // range values, so the reconstructed step picks up floating-point dust + // (e.g. 9.9999711 instead of 10). Snap log10(step) to the nearest + // integer when it's right at a 10^k boundary so we bucket correctly. + const step = + (labeledRangeValues[1] as number) - (labeledRangeValues[0] as number); + const log = Math.log10(Math.abs(step)); + const exponent = + Math.abs(log - Math.round(log)) < 1e-3 + ? Math.round(log) + : Math.floor(log); + const mantissa = Math.abs(step) / Math.pow(10, exponent); + expect([1, 2, 5]).toContain(Math.round(mantissa)); + + const eps = Math.abs(step) * 1e-3; + labeledRangeValues.forEach((v) => { + expect(Math.abs(v - Math.round(v / step) * step)).toBeLessThan(eps); + }); + }); + + it("treats forceTickCount as a hint when domain is normalized to [0, 1]", () => { + // Production-like: domain is [0, 1], range is the actual data range. + // The labeled display values must be d3-nice in range space. + const params = { + displayType: QuestionType.Numeric, + axisLength: 400, + direction: ScaleDirection.Vertical, + domain: [0, 1] as [number, number], + zoomedDomain: [0, 1] as [number, number], + scaling: { + range_min: 0, + range_max: 100, + zero_point: null, + }, + forceTickCount: 5, + }; + + const scale = generateScale(params); + + const labeledDomainTicks = scale.ticks.filter( + (t) => scale.tickFormat(t) !== "" + ); + const labeledRangeValues = labeledDomainTicks.map((t) => 0 + t * 100); + + // forceTickCount is treated as a ceiling, not a hint. d3 can't fit 5 + // ticks across [0, 100] with a step in {1, 2, 5} * 10^k, so we + // accept a coarser result like [0, 50, 100] (step 50, 3 ticks). + expect(labeledRangeValues.length).toBeGreaterThanOrEqual(2); + expect(labeledRangeValues.length).toBeLessThanOrEqual(5); + const step = + (labeledRangeValues[1] as number) - (labeledRangeValues[0] as number); + const log = Math.log10(Math.abs(step)); + const exponent = + Math.abs(log - Math.round(log)) < 1e-3 + ? Math.round(log) + : Math.floor(log); + const mantissa = Math.abs(step) / Math.pow(10, exponent); + expect([1, 2, 5]).toContain(Math.round(mantissa)); + }); + + it("treats forceTickCount as a ceiling, not a soft hint", () => { + // Regression for the [-10, 8] feed-card case: d3.ticks(-10, 8, 3) + // picks step 2 and returns 10 ticks, which overflowed the small + // axis and produced overlapping labels. The ceiling guarantees + // the labeled count never exceeds forceTickCount. + const params = { + displayType: QuestionType.Numeric, + axisLength: 150, + direction: ScaleDirection.Vertical, + domain: [0, 1] as [number, number], + zoomedDomain: [0, 1] as [number, number], + scaling: { + range_min: -10, + range_max: 8, + zero_point: null, + }, + forceTickCount: 3, + }; + + const scale = generateScale(params); + const labeledTicks = scale.ticks.filter( + (t) => scale.tickFormat(t) !== "" + ); + + expect(labeledTicks.length).toBeLessThanOrEqual(3); + expect(labeledTicks.length).toBeGreaterThanOrEqual(2); + }); + + it("does not throw when forceTickCount is 1 on a date axis", () => { + // Edge case: forceTickCount === 1 used to produce NaN ticks via i/0. + const params = { + displayType: QuestionType.Date, + axisLength: 200, + direction: ScaleDirection.Vertical, + domain: [0, 1] as [number, number], + zoomedDomain: [0, 1] as [number, number], + scaling: { + range_min: 1678838400, + range_max: 1778803200, + zero_point: null, + }, + forceTickCount: 1, + }; + + const scale = generateScale(params); + scale.ticks.forEach((t) => expect(Number.isNaN(t)).toBe(false)); + }); }); }); diff --git a/front_end/src/utils/charts/axis.ts b/front_end/src/utils/charts/axis.ts index be42e7e11d..4b2588b452 100644 --- a/front_end/src/utils/charts/axis.ts +++ b/front_end/src/utils/charts/axis.ts @@ -301,62 +301,6 @@ export function generateTimestampXScale( }; } -/** - * Takes an array of values and rounds them to the minimum - * number of significant digits such that no two values - * are rounded to the same value. - * Values must be sorted in ascending order. - */ -function minimumSignificantRounding(values: number[]): number[] { - const roundedValues: number[] = []; - const EPS = 1e-12; - - function sigfigRound(val: number, sigfigs: number): number { - if (val === 0) return 0; // Special case for zero - const divisor = 10 ** (sigfigs - Math.floor(Math.log10(Math.abs(val))) - 1); - return Math.round(val * divisor) / divisor; - } - // TODO: more intelligent ordering for rounded tick value selection - // TODO: add dextrous rounding reflecting sig fig cost algorithm - values.forEach((value, i) => { - if (i === 0 || i === values.length - 1) { - roundedValues.push(value); - return; - } - const prevValue = values[i - 1]; - const nextValue = values[i + 1]; - - if (prevValue == null || nextValue == null) { - roundedValues.push(value); - return; - } - - // flat/duplicate guards - if ( - Math.abs(value - prevValue) < EPS || - Math.abs(nextValue - value) < EPS - ) { - roundedValues.push(value); - return; - } - - let candidate = value; - for (let digits = 1; digits <= 12; digits++) { - candidate = sigfigRound(value, digits); - const denom = value - prevValue; - if ( - Math.abs(denom) < EPS || - Math.abs((value - candidate) / denom) < 0.2 - ) { - break; - } - } - roundedValues.push(candidate); - }); - - return roundedValues; -} - function getSigFigCost(value: number, logarithmic: boolean = false): number { const absValue = Math.abs(value); // take the length of mantissa of the exponential rounded @@ -399,6 +343,49 @@ function getSigFigCost(value: number, logarithmic: boolean = false): number { return mantissa.length; } +/** + * Like d3.ticks, but treats the count as a ceiling instead of a hint. + * d3.ticks picks the nicest step size and returns however many ticks + * fit — which can exceed the requested count and overflow tight axes. + * Walking down from maxCount until the result fits guarantees count + * <= maxCount while preserving the {1,2,5} * 10^k step guarantee. + */ +function niceTicksAtMost( + start: number, + stop: number, + maxCount: number +): number[] { + for (let c = Math.max(1, maxCount); c >= 1; c--) { + const t = d3.ticks(start, stop, c); + if (t.length >= 2 && t.length <= maxCount) return t; + } + // No c produced a count in [2, maxCount] — typically because the nice + // step sizes near maxCount land us at 1 tick on one side and >maxCount + // on the other. Try counts up to the global cap (4) looking for any + // nice result with at least 2 ticks; better to slightly exceed the + // local cap than fall back to ugly raw endpoints. + for (let c = 1; c <= 4; c++) { + const t = d3.ticks(start, stop, c); + if (t.length >= 2 && t.length <= 4) return t; + } + return start === stop ? [start] : [start, stop]; +} + +/** + * Returns a domain that contains both the original data domain and every + * tick in the supplied array. Use to widen Victory's yDomain so that + * generateScale's tick labels actually land inside the chart's drawing + * area — important for log-warped questions where the data clusters in + * a small slice and the auto-zoomed yDomain would otherwise clip ticks. + */ +export function widenDomainToTicks( + domain: Tuple, + ticks: number[] +): Tuple { + if (ticks.length === 0) return domain; + return [Math.min(domain[0], ...ticks), Math.max(domain[1], ...ticks)]; +} + /** * Take a range's min and max and finds the tick spacing that minimizes * the average number of significant digits in the tick values. @@ -619,79 +606,125 @@ export function generateScale({ majorTicks.push(minorTicks.at(-1) ?? 1); } else if (isNil(zeroPoint)) { // Linear Scaling - // Typical scaling, evenly spaced ticks - // choose optimal tick count to minimize the number - // of significant digits in the tick labels - const majorTickCount = forceTickCount - ? forceTickCount - : findOptimalTickCount(rangeMin, rangeMax, 4, maxLabelCount); - majorTicks = range(0, majorTickCount).map( - (i) => - Math.round( - (zoomedDomainMin + - (i / (majorTickCount - 1)) * (zoomedDomainMax - zoomedDomainMin)) * - 1000000 - ) / 1000000 - ); - const minorTicksPerMajor = findOptimalTickCount( - rangeMin, - rangeMin + (rangeMax - rangeMin) * (majorTicks[1] ?? 1 / majorTickCount), - direction === "horizontal" ? 4 : 2, - direction === "horizontal" ? 10 : 5 - ); - const minorTickCount = forceTickCount - ? forceTickCount - : (majorTickCount - 1) * minorTicksPerMajor + 1; - minorTicks = range(0, minorTickCount).map( - (i) => - Math.round( - (zoomedDomainMin + - (i / (minorTickCount - 1)) * (zoomedDomainMax - zoomedDomainMin)) * - 1000000 - ) / 1000000 - ); - } else { - // Logarithmic Scaling - // Labeled ticks are not spaced evenly, but rather rounded to the nearby - // values that have the fewest significant digits - // Then, minor ticks are spaced evenly in real space, showcasing the - // strength of the logarithmic scaling - const minLabelCount = forceTickCount ?? Math.ceil(maxLabelCount / 2) + 1; - let bestTicks: number[] = []; - let bestAvgDigits = Infinity; - for (let i = maxLabelCount; i >= minLabelCount; i--) { - const unscaledTargets = Array.from( - { length: i }, - (_, j) => - zoomedDomainMin + - ((zoomedDomainMax - zoomedDomainMin) * (j * 1)) / (i - 1) + if (displayType === QuestionType.Numeric) { + // Pick mathematically "nice" ticks (multiples of {1,2,5} * 10^k) in + // the actual data range, then map them back to domain coordinates. + // Doing this in range space matters when domain is normalized [0, 1] + // but the data range is something like [-27.7, 20]: nice values in + // domain space unscale to ugly display values, so we have to compute + // niceness against the values users will see. + // forceTickCount, when supplied, is treated as a hint to d3.ticks(); + // the resulting count may differ by ±1-2 in exchange for nicer values. + const tickCountHint = Math.min(4, forceTickCount ?? maxLabelCount); + const zoomedRangeMin = scaleInternalLocation( + unscaleNominalLocation(zoomedDomainMin, domainScaling), + rangeScaling ); - const scaledTargets = unscaledTargets.map((x) => - scaleInternalLocation(x, rangeScaling) + const zoomedRangeMax = scaleInternalLocation( + unscaleNominalLocation(zoomedDomainMax, domainScaling), + rangeScaling ); - const roundedScaledTargets = minimumSignificantRounding(scaledTargets); - const sigFigCosts = roundedScaledTargets.map((x) => - getSigFigCost(x, true) + + const niceMajorRangeTicks = niceTicksAtMost( + zoomedRangeMin, + zoomedRangeMax, + tickCountHint ); - const avgDigits = sigFigCosts.reduce((sum, cost) => sum + cost, 0) / i; - if (avgDigits < bestAvgDigits) { - bestAvgDigits = avgDigits; - bestTicks = roundedScaledTargets; - } + const rangeToDomain = (v: number) => + scaleInternalLocation( + unscaleNominalLocation(v, rangeScaling), + domainScaling + ); + majorTicks = niceMajorRangeTicks.map( + (v) => Math.round(rangeToDomain(v) * 1000000) / 1000000 + ); + + // Minor tick density is based on the major step in range units, not + // an absolute tick position. + const majorRangeStep = + niceMajorRangeTicks.length >= 2 + ? (niceMajorRangeTicks[1] as number) - + (niceMajorRangeTicks[0] as number) + : zoomedRangeMax - zoomedRangeMin; + const minorTicksPerMajor = findOptimalTickCount( + 0, + majorRangeStep, + direction === "horizontal" ? 4 : 2, + direction === "horizontal" ? 10 : 5 + ); + const minorTickCount = + Math.max(majorTicks.length - 1, 1) * minorTicksPerMajor + 1; + const denseMinor = d3 + .ticks(zoomedRangeMin, zoomedRangeMax, minorTickCount) + .map((v) => Math.round(rangeToDomain(v) * 1000000) / 1000000); + // Major ticks must always be a subset of minor — otherwise their + // labels get filtered out at render time (tickFormat checks major + // membership). The d3.ticks count for minor can lock onto a step + // that doesn't include the major positions, so merge explicitly. + minorTicks = Array.from(new Set([...majorTicks, ...denseMinor])).sort( + (a, b) => a - b + ); + } else if (forceTickCount) { + // Non-numeric linear scales (e.g. date axes) need exact, evenly-spaced + // ticks across the zoomed domain — d3.ticks would produce ugly raw + // timestamps. Clamp count to >= 2 to avoid divide-by-zero. + const count = Math.max(2, forceTickCount); + const evenlySpaced = range(0, count).map( + (i) => + Math.round( + (zoomedDomainMin + + (i / (count - 1)) * (zoomedDomainMax - zoomedDomainMin)) * + 1000000 + ) / 1000000 + ); + majorTicks = evenlySpaced; + minorTicks = evenlySpaced.slice(); + } else { + const count = Math.max(2, maxLabelCount); + const evenlySpaced = range(0, count).map( + (i) => + Math.round( + (zoomedDomainMin + + (i / (count - 1)) * (zoomedDomainMax - zoomedDomainMin)) * + 1000000 + ) / 1000000 + ); + majorTicks = evenlySpaced; + minorTicks = evenlySpaced.slice(); } - majorTicks = bestTicks.map( + } else { + // Logarithmic Scaling + // Pick nice round numbers in display (range) space, then unscale each + // back to domain coordinates so they land at the right positions on + // the warped axis. The previous approach picked evenly-spaced warped + // positions and rounded them to fewest sig figs, but kept the + // endpoints verbatim — which produced ugly labels like 52.7. + const tickCountHint = Math.min(4, forceTickCount ?? maxLabelCount); + const displayMin = scaleInternalLocation(zoomedDomainMin, rangeScaling); + const displayMax = scaleInternalLocation(zoomedDomainMax, rangeScaling); + const niceMajorRangeTicks = niceTicksAtMost( + Math.min(displayMin, displayMax), + Math.max(displayMin, displayMax), + tickCountHint + ); + + majorTicks = niceMajorRangeTicks.map( (x) => Math.round(unscaleNominalLocation(x, rangeScaling) * 1000000) / 1000000 ); + // Minor ticks subdivide each major interval evenly in display space, + // then unscale to domain — that's what makes the gridlines appear + // logarithmically spaced and showcases the warp. const tickCount = forceTickCount ? forceTickCount : (maxLabelCount - 1) * (direction === "horizontal" ? 10 : 3) + 1; - const minorTicksPerMajorInterval = (tickCount - 1) / (maxLabelCount - 1); + const minorTicksPerMajorInterval = + (tickCount - 1) / Math.max(1, niceMajorRangeTicks.length - 1); minorTicks = majorTicks.slice(); - range(0, bestTicks.length - 1).forEach((i) => { - const prevMajor = bestTicks.at(i) ?? 0; - const nextMajor = bestTicks.at(i + 1) ?? 1; + range(0, niceMajorRangeTicks.length - 1).forEach((i) => { + const prevMajor = niceMajorRangeTicks.at(i) ?? 0; + const nextMajor = niceMajorRangeTicks.at(i + 1) ?? 1; const step = (nextMajor - prevMajor) / minorTicksPerMajorInterval; for (let j = 0; j < minorTicksPerMajorInterval - 1; j++) { const newMinorTick = prevMajor + (j + 1) * step; @@ -867,7 +900,11 @@ export function generateScale({ // } return { - ticks: minorTicks, + // alwaysShowTicks tells the chart to label every tick value verbatim + // (it bypasses the major/minor filter in tickFormat). Returning the + // major array honors the cap-of-4 in that case; returning the dense + // minor array would let callers like group_chart blow past the cap. + ticks: alwaysShowTicks ? majorTicks : minorTicks, tickFormat: tickFormat, cursorFormat: cursorFormat, };