From edc4fee4d239d78b907e54c683577a34419623f4 Mon Sep 17 00:00:00 2001 From: Jaladh Singhal Date: Thu, 8 Oct 2026 18:17:05 -0700 Subject: [PATCH] FIREFLY-2117: prevent spectral lines from stretching the chart's x range --- src/firefly/js/charts/ChartUtil.js | 68 ++++++++++++++++- .../js/charts/__tests__/ChartUtil-test.js | 74 ++++++++++++++++++- src/firefly/js/charts/ui/PlotlyChartArea.jsx | 8 +- 3 files changed, 146 insertions(+), 4 deletions(-) diff --git a/src/firefly/js/charts/ChartUtil.js b/src/firefly/js/charts/ChartUtil.js index e651dff326..4c134c8a8a 100644 --- a/src/firefly/js/charts/ChartUtil.js +++ b/src/firefly/js/charts/ChartUtil.js @@ -645,6 +645,13 @@ export function flattenAnnotations(annotations) { } const SHAPE_HOVER_STEPS = 10; // points per line stacked along the shape's height, for a hover target that tracks the cursor vertically +const SHAPES_AUTORANGE_PAD = 0.05; // fraction of the data span padded on each side of an autorange clip, standing in for Plotly's own padding + +/** + * vertical line at a data x, with y in 'paper' (0-1 of plot area height) units. E.g. a spectral line + * @param shape + */ +const isVerticalPaperLine = (shape) => shape?.type === 'line' && shape.xref === 'x' && shape.yref === 'paper' && shape.x0 === shape.x1; /** * Builds companion hover trace(s) for Plotly shapes that carry a `hovertext`. @@ -667,7 +674,7 @@ const SHAPE_HOVER_STEPS = 10; // points per line stacked along the shape's heigh * (the 'y2' axis they're plotted on) to merge in alongside them — empty/{} when there's nothing hoverable */ export function makeShapeHoverTrace(shapes=[]) { - const hoverable = shapes.filter((s) => s?.type === 'line' && s.yref === 'paper' && s.hovertext); + const hoverable = shapes.filter((shape) => isVerticalPaperLine(shape) && shape.hovertext); if (hoverable.length === 0) return {traces: [], layout: {}}; const byGroup = new Map(); @@ -699,6 +706,65 @@ export function makeShapeHoverTrace(shapes=[]) { return {traces, layout: {yaxis2: {overlaying: 'y', range: [0, 1], visible: false, fixedrange: true}}}; } +/** + * Builds the layout fragment that keeps shapes from widening axis autorange beyond the data. + * + * Plotly counts data-referenced shapes (and their hover traces from makeShapeHoverTrace) in autorange, with no + * per-shape opt-out, so shapes far outside the data, squeeze plotted data into a sliver. This clips the affected axis' + * autorange (`autorangeoptions.clipmin/clipmax`) to the padded data extent: autorange stays on, so the 1x button and + * filter/column resets keep working, and shapes beyond it are still drawn when zoomed out. An explicit range (e.g. + * user-set min/max) turns autorange off, so the clip doesn't apply then. Not specific to any one feature. + * + * NOTE: Only handles vertical line shapes with paper y (`xref:'x', yref:'paper', x0=x1`), which only affect x. + * + * @param {Array} data - chart traces to fit (not selected/highlighted/hover traces) + * @param {object} layout - chart layout; uses `shapes` and the axis `type` ('log' pads in log space) + * @returns {object} {} for a chart without `shapes`, otherwise a fragment to deep-merge into the plotly layout, e.g. + * `{xaxis: {autorangeoptions: {clipmin, clipmax}}}`, with null bounds when there's nothing to clip. + */ +export function makeShapesAutorangeLayout(data=[], layout={}) { + if (!layout.shapes) return {}; + const clip = layout.shapes.some(isVerticalPaperLine) ? getDataXExtentClip(data, layout.xaxis?.type === 'log') : undefined; + // null, not undefined, clears a clip Plotly already has: PlotlyWrapper's relayout only sends keys present here + return {xaxis: {autorangeoptions: {clipmin: clip?.clipmin ?? null, clipmax: clip?.clipmax ?? null}}}; +} + +/** + * @param {Array} data - chart traces; hidden ones are skipped, x error bars included + * @param {boolean} isLog - pad in log space and skip x <= 0 + * @returns {{clipmin: number, clipmax: number}|undefined} padded x extent, undefined when it's empty or zero-width + */ +function getDataXExtentClip(data, isLog) { + let min = Infinity, max = -Infinity; + const addX = (xVal) => { + if (!Number.isFinite(xVal) || (isLog && xVal <= 0)) return; + if (xVal < min) min = xVal; + if (xVal > max) max = xVal; + }; + + data.filter((trace) => trace?.visible !== false && trace?.visible !== 'legendonly').forEach((trace) => { + const xVals = trace.x ?? []; + const err = trace.error_x?.visible !== false ? trace.error_x : undefined; + const errPlus = err?.array; + const errMinus = err?.symmetric === false ? err?.arrayminus : errPlus; + for (let idx = 0; idx < xVals.length; idx++) { + const xVal = parseFloat(xVals[idx]); // NaN (skipped) for null/undefined/'' + addX(xVal); + if (errPlus) addX(xVal + parseFloat(errPlus[idx])); + if (errMinus) addX(xVal - parseFloat(errMinus[idx])); + } + }); + if (!(max > min)) return undefined; + + if (isLog) { + const [logMin, logMax] = [Math.log10(min), Math.log10(max)]; + const pad = SHAPES_AUTORANGE_PAD * (logMax - logMin); + return {clipmin: 10 ** (logMin - pad), clipmax: 10 ** (logMax + pad)}; + } + const pad = SHAPES_AUTORANGE_PAD * (max - min); + return {clipmin: min - pad, clipmax: max + pad}; +} + export function updateSelection(chartId, selectInfo) { const {data, activeTrace=0} = getChartData(chartId); diff --git a/src/firefly/js/charts/__tests__/ChartUtil-test.js b/src/firefly/js/charts/__tests__/ChartUtil-test.js index 344f414dda..51c9e7bc4c 100644 --- a/src/firefly/js/charts/__tests__/ChartUtil-test.js +++ b/src/firefly/js/charts/__tests__/ChartUtil-test.js @@ -1,5 +1,5 @@ /*eslint-env node, jest */ -import {formatColExpr} from 'firefly/charts/ChartUtil.js'; +import {formatColExpr, makeShapeHoverTrace, makeShapesAutorangeLayout} from 'firefly/charts/ChartUtil.js'; describe('ChartUtil', () => { @@ -12,3 +12,75 @@ describe('ChartUtil', () => { expect(formatColExpr({colOrExpr: '1e5*e', quoted: true, colNames: ['e']})).toBe('1e5*"e"'); }); }); + +const vLine = (xPos, extra={}) => ({type: 'line', x0: xPos, x1: xPos, y0: 0, y1: 1, xref: 'x', yref: 'paper', ...extra}); + +describe('makeShapeHoverTrace', () => { + test('builds a hover trace only for vertical paper-y lines with hovertext', () => { + const shapes = [ + vLine(5, {hovertext: 'a'}), + vLine(6), // no hovertext + {type: 'line', x0: 1, x1: 2, y0: 0, y1: 1, xref: 'x', yref: 'paper', hovertext: 'slanted'}, + ]; + const {traces} = makeShapeHoverTrace(shapes); + expect(traces).toHaveLength(1); + expect(traces[0].x.filter((xVal) => xVal !== null)).toEqual(Array(10).fill(5)); + }); +}); + +describe('makeShapesAutorangeLayout', () => { + const data = [{x: [10, 20]}]; + const noClip = {xaxis: {autorangeoptions: {clipmin: null, clipmax: null}}}; + + test('returns {} for a chart without shapes', () => { + expect(makeShapesAutorangeLayout(data, {})).toEqual({}); + expect(makeShapesAutorangeLayout(data, {xaxis: {type: 'log'}})).toEqual({}); + }); + + test('returns null bounds (to clear a previous clip) without vertical paper-y line shapes', () => { + expect(makeShapesAutorangeLayout(data, {shapes: []})).toEqual(noClip); + expect(makeShapesAutorangeLayout(data, {shapes: [{type: 'line', x0: 0, x1: 100, y0: 0.5, y1: 0.5, xref: 'x', yref: 'paper'}]})).toEqual(noClip); + expect(makeShapesAutorangeLayout(data, {shapes: [{type: 'rect', x0: 0, x1: 100, y0: 0, y1: 1, xref: 'x', yref: 'paper'}]})).toEqual(noClip); + expect(makeShapesAutorangeLayout(data, {shapes: [vLine(0.5, {xref: 'paper'})]})).toEqual(noClip); + }); + + test('clips to the padded data x extent across traces, with or without hovertext on shapes', () => { + expect(makeShapesAutorangeLayout([{x: [12, 20]}, {x: [10, 15]}], {shapes: [vLine(500)]})) + .toEqual({xaxis: {autorangeoptions: {clipmin: 9.5, clipmax: 20.5}}}); + expect(makeShapesAutorangeLayout(data, {shapes: [vLine(500, {hovertext: 'a'})]})) + .toEqual({xaxis: {autorangeoptions: {clipmin: 9.5, clipmax: 20.5}}}); + }); + + test('widens the extent by x error bars', () => { + const symmetric = [{x: [10, 20], error_x: {array: [1, 2]}}]; + expect(makeShapesAutorangeLayout(symmetric, {shapes: [vLine(500)]})) // extent [9, 22], pad 0.65 + .toEqual({xaxis: {autorangeoptions: {clipmin: expect.closeTo(8.35), clipmax: expect.closeTo(22.65)}}}); + + const asymmetric = [{x: [10, 20], error_x: {symmetric: false, array: [0, 4], arrayminus: [4, 0]}}]; + expect(makeShapesAutorangeLayout(asymmetric, {shapes: [vLine(500)]})) // extent [6, 24], pad 0.9 + .toEqual({xaxis: {autorangeoptions: {clipmin: expect.closeTo(5.1), clipmax: expect.closeTo(24.9)}}}); + + const hiddenErr = [{x: [10, 20], error_x: {visible: false, array: [5, 5]}}]; + expect(makeShapesAutorangeLayout(hiddenErr, {shapes: [vLine(500)]})) + .toEqual({xaxis: {autorangeoptions: {clipmin: 9.5, clipmax: 20.5}}}); + }); + + test('ignores hidden traces and non-finite x values', () => { + const traces = [{x: [10, NaN, null, 20]}, {x: [-100, 100], visible: 'legendonly'}, {x: [-100, 100], visible: false}]; + expect(makeShapesAutorangeLayout(traces, {shapes: [vLine(500)]})) + .toEqual({xaxis: {autorangeoptions: {clipmin: 9.5, clipmax: 20.5}}}); + }); + + test('pads in log space and skips x <= 0 on a log axis', () => { + const {clipmin, clipmax} = makeShapesAutorangeLayout([{x: [-1, 0, 1, 100]}], {shapes: [vLine(5000)], xaxis: {type: 'log'}}) + .xaxis.autorangeoptions; + expect(Math.log10(clipmin)).toBeCloseTo(-0.1); + expect(Math.log10(clipmax)).toBeCloseTo(2.1); + }); + + test('returns null bounds when data has no finite x extent of non-zero width', () => { + expect(makeShapesAutorangeLayout([{x: [NaN, null]}], {shapes: [vLine(500)]})).toEqual(noClip); + expect(makeShapesAutorangeLayout([{x: [7, 7]}], {shapes: [vLine(500)]})).toEqual(noClip); + expect(makeShapesAutorangeLayout([], {shapes: [vLine(500)]})).toEqual(noClip); + }); +}); diff --git a/src/firefly/js/charts/ui/PlotlyChartArea.jsx b/src/firefly/js/charts/ui/PlotlyChartArea.jsx index 11aa474fb6..b6e199304e 100644 --- a/src/firefly/js/charts/ui/PlotlyChartArea.jsx +++ b/src/firefly/js/charts/ui/PlotlyChartArea.jsx @@ -1,12 +1,12 @@ import React, {useEffect, useRef} from 'react'; import PropTypes from 'prop-types'; -import {get, set, cloneDeep, isArray} from 'lodash'; +import {get, set, cloneDeep, isArray, merge} from 'lodash'; import {PlotlyWrapper} from './PlotlyWrapper.jsx'; import {showInfoPopup} from '../../ui/PopupUtil.jsx'; import {dispatchChartHighlighted, dispatchChartUpdate, dispatchSetActiveTrace, getAnnotations, getChartData, usePlotlyReact} from '../ChartsCntlr.js'; import {clearChartConn, flattenAnnotations, handleTableSourceConnections, isGroupedChart, isScatter2d, - makeShapeHoverTrace} from '../ChartUtil.js'; + makeShapeHoverTrace, makeShapesAutorangeLayout} from '../ChartUtil.js'; import {useStoreConnector} from 'firefly/ui/SimpleComponent.jsx'; import {Skeleton, useTheme} from '@mui/joy'; @@ -77,10 +77,14 @@ export function PlotlyChartArea({chartId, widthPx, heightPx, thumbnail}) { const {traces: hoverTraces, layout: hoverLayout} = makeShapeHoverTrace(layout.shapes); pdata = pdata.concat(hoverTraces); + // keep shapes (e.g. spectral lines) and their hover traces from widening autorange beyond the data + const shapesAutorangeLayout = makeShapesAutorangeLayout(data, layout); + const {chartWidth, chartHeight} = calculateChartSize(widthPx, heightPx, xyratio, stretch); const showlegend = isGroupedChart(chartId) ? true : (layout?.showlegend ?? data.length > 1); const playout = cloneDeep({showlegend, ...adjustLayout(layout, theme), width: chartWidth, height: chartHeight, annotations, ...hoverLayout}); + merge(playout, shapesAutorangeLayout); // deep merge (in place): it adds keys inside existing axes const style = {float: 'left'}; if (chartWidth > widthPx || chartHeight > heightPx) {