From 4bd72306a2390cf11ece6ae18aa1faf84e84fdf9 Mon Sep 17 00:00:00 2001 From: Jaladh Singhal Date: Fri, 2 Oct 2026 17:24:03 -0700 Subject: [PATCH] FIREFLY-2103: Fix reverse axis with limits not working FIREFLY-2103: fix reversing chart axis when min/max bounds are set adjustAxesRange rebuilt the range from the user's bounds but took the axis direction from the old layout range, discarding the reverse/un-reverse just requested in the options dialog. Take the direction from the reverse option's changes (re-ordered range or autorange), falling back to the current layout only when the fields have no reverse option. Also simplify the range re-ordering and drop lodash reverse. FIREFLY-2103: centralize chart axis range evaluation Replace the two-pass axis range logic in evalChangesFromFields (reverse option reordering the range, then adjustAxesRange inferring direction back from it) with evalAxisRangeChanges in AxisRangeUtil.js, which decides range and autorange per axis directly from the requested reverse/log options, bounds, column-change reset and current range. Behavior is unchanged except: staying on log no longer rebuilds the range and turns off autoscale on every Apply, and switching to log with a current range reaching 0 or below falls back to autorange instead of a NaN range. Add AxisRange-test.js covering reverse, bounds, column change, log scale and callers without axis options. FIREFLY-2103: autoscale when a single axis bound has nothing to fill the other from With one Y/X bound and no current range to fill the other end from (axis column changed in the same Apply, or a new chart), the range was built with a NaN end, which Plotly drew as an arbitrary span (e.g. 69.05 to 690.5). Fall back to autoscale in the requested direction instead. --- src/firefly/js/charts/AxisRangeUtil.js | 84 +++++++ .../js/charts/__tests__/AxisRange-test.js | 212 ++++++++++++++++++ .../js/charts/ui/options/BasicOptions.jsx | 84 +------ 3 files changed, 304 insertions(+), 76 deletions(-) create mode 100644 src/firefly/js/charts/AxisRangeUtil.js create mode 100644 src/firefly/js/charts/__tests__/AxisRange-test.js diff --git a/src/firefly/js/charts/AxisRangeUtil.js b/src/firefly/js/charts/AxisRangeUtil.js new file mode 100644 index 0000000000..ebcd8a9422 --- /dev/null +++ b/src/firefly/js/charts/AxisRangeUtil.js @@ -0,0 +1,84 @@ +/* + * License information at https://github.com/Caltech-IPAC/firefly/blob/master/License.txt + */ + +/** + * Whether a plotly axis is reversed. + * Plotly keeps the direction as autorange 'reversed' or as the order of the range bounds (range[0] > range[1]). + * @param {object} axisLayout - plotly layout of the axis + * @returns {boolean} + */ +export function isAxisReversed(axisLayout) { + const autorange = axisLayout?.autorange; + const range = axisLayout?.range || []; + return (autorange === 'reversed') || (range[1] < range[0]); +} + +/** + * Get range for a plotly axis + * Plotly requires range to be reversed if the axis is reversed, + * and limits to be log if axis scale is log + * @param min - minimum value + * @param max - maximum value + * @param isLog - true, if an axis uses log scale + * @param isReversed - true, if the axis should be reversed + * @returns {Array} an array for axis range property in plotly layout + */ +export function getRange(min, max, isLog, isReversed) { + const [r1, r2] = isReversed ? [max, min] : [min, max]; + return isLog ? [Math.log10(r1), Math.log10(r2)] : [r1, r2]; +} + +/** + * Evaluate the plotly range changes of one axis from the chart options fields. + * + * Everything is decided from what the user asked for in the fields (reverse/log options, bounds, axis column change), + * with the current axis layout used only for the current range and for whatever the fields don't specify. + * + * @param {object} p + * @param {string} p.a - axis: 'x' or 'y' + * @param {object} p.fields - chart options fields (fieldKey -> value) + * @param {object} [p.axisLayout] - current plotly layout of the axis + * @returns {object} changes for `layout.axis.range` and `layout.axis.autorange`; + * empty when the axis range should be left as is + */ +export function evalAxisRangeChanges({a, fields, axisLayout={}}) { + const rangeKey = `layout.${a}axis.range`; + const autorangeKey = `layout.${a}axis.autorange`; + + const hasOpts = `__${a}options` in fields; + const opts = fields[`__${a}options`] || ''; + const isCurrLog = axisLayout.type === 'log'; + const flip = hasOpts ? opts.includes('flip') : isAxisReversed(axisLayout); + const isLog = hasOpts ? opts.includes('log') : isCurrLog; + const scaleChanged = isLog !== isCurrLog; // log range is in log10 units, so a switch needs a rebuild + const reset = Boolean(fields[`__${a}reset`]); + let min = parseFloat(fields[`fireflyLayout.${a}axis.min`]); + let max = parseFloat(fields[`fireflyLayout.${a}axis.max`]); + const hasBounds = !Number.isNaN(min) || !Number.isNaN(max); + + const currRange = !reset && axisLayout.range; // reset: axis column changed, current range doesn't apply + const autoscale = {[autorangeKey]: flip ? 'reversed' : true, [rangeKey]: undefined}; + + if (!hasBounds) { + if (!hasOpts) return {}; // caller doesn't manage this axis + if (!currRange) return autoscale; // nothing to keep + if (!scaleChanged) { // keep the current range, reordered to the requested direction + const [r0, r1] = currRange; + const needsSwap = flip ? r0 < r1 : r1 < r0; + return needsSwap ? {[rangeKey]: [r1, r0], [autorangeKey]: false} : {[rangeKey]: currRange}; + } + // scale switch: falls through, both bounds are taken from the current range below + } + + // fill missing bounds from the current range, in data units + if (currRange && hasOpts) { + const [d0, d1] = isCurrLog ? currRange.map((e) => 10 ** e) : currRange; + if (Number.isNaN(min)) min = Math.min(d0, d1); + if (Number.isNaN(max)) max = Math.max(d0, d1); + } + + if (Number.isNaN(min) || Number.isNaN(max)) return autoscale; // a bound with nothing to fill it from + if (isLog && (min <= 0 || max <= 0)) return autoscale; // can't show 0 or below on a log scale + return {[rangeKey]: getRange(min, max, isLog, flip), [autorangeKey]: false}; +} diff --git a/src/firefly/js/charts/__tests__/AxisRange-test.js b/src/firefly/js/charts/__tests__/AxisRange-test.js new file mode 100644 index 0000000000..6aa1893cb1 --- /dev/null +++ b/src/firefly/js/charts/__tests__/AxisRange-test.js @@ -0,0 +1,212 @@ +/*eslint-env node, jest */ + +// PlotlyToolbar pulls in canvas-based UI that jsdom can't load; BasicOptions only needs ActiveTraceSelect from it +jest.mock('firefly/charts/ui/PlotlyToolbar.jsx', () => ({ActiveTraceSelect: () => null})); +jest.mock('firefly/charts/ChartsCntlr.js', () => ({ + ...jest.requireActual('firefly/charts/ChartsCntlr.js'), + getChartData: jest.fn(), +})); + +import {getChartData} from 'firefly/charts/ChartsCntlr.js'; +import {evalChangesFromFields} from 'firefly/charts/ui/options/BasicOptions.jsx'; +import {evalAxisRangeChanges} from 'firefly/charts/AxisRangeUtil.js'; + +/** + * Pick the axis range related keys present in the changes, distinguishing "not changed" (key absent) from "set to undefined" + * @param {object} changes + * @param {string} a - axis: 'x' or 'y' + * @returns {object} + */ +function axisKeys(changes, a) { + const out = {}; + ['range', 'autorange', 'type'].forEach((p) => { + const k = `layout.${a}axis.${p}`; + if (k in changes) out[p] = changes[k]; + }); + return out; +} + +/** + * Run evalChangesFromFields against a chart with the given stored layout and pick out its axis range keys + * @param {object} layout - chart layout in the store (as synced from plotly) + * @param {object} fields - options dialog fields + * @returns {{x: object, y: object, changes: object}} per-axis range keys, plus all changes + */ +function evalChangesFromFieldsAxes(layout, fields) { + getChartData.mockReturnValue({layout, data: []}); + const changes = evalChangesFromFields('test-chart', undefined, fields); + return {x: axisKeys(changes, 'x'), y: axisKeys(changes, 'y'), changes}; +} + +const AUTO = {range: [1, 9], autorange: true}; // initial chart: plotly-computed range synced into the store +const FIXED = {range: [1, 9], autorange: false}; // e.g. after a previous Apply with bounds +const REV = {range: [9, 1], autorange: false}; // reversed +const ZOOMED = {range: [3, 4], autorange: false}; // after a zoom +const LOG = {type: 'log', range: [0, 1], autorange: true}; // log axis showing 1..10 (range is in log10 units) + +const bounds = (min, max) => ({ + ...(min !== undefined && {'fireflyLayout.yaxis.min': `${min}`}), + ...(max !== undefined && {'fireflyLayout.yaxis.max': `${max}`}), +}); + +describe('evalChangesFromFields: axis range changes', () => { + + describe('reverse without bounds', () => { + test('unchanged direction keeps the range and leaves autorange alone', () => { + expect(evalChangesFromFieldsAxes({yaxis: AUTO}, {__yoptions: ''}).y).toEqual({range: [1, 9]}); + expect(evalChangesFromFieldsAxes({yaxis: REV}, {__yoptions: 'flip'}).y).toEqual({range: [9, 1]}); + }); + test('reverse swaps the kept range and fixes it', () => { + expect(evalChangesFromFieldsAxes({yaxis: AUTO}, {__yoptions: 'flip'}).y) + .toEqual({range: [9, 1], autorange: false}); + }); + test('un-reverse swaps the kept range back', () => { + expect(evalChangesFromFieldsAxes({yaxis: REV}, {__yoptions: ''}).y) + .toEqual({range: [1, 9], autorange: false}); + }); + test('no stored range uses autorange', () => { + expect(evalChangesFromFieldsAxes({}, {__yoptions: ''}).y).toEqual({autorange: true, range: undefined}); + expect(evalChangesFromFieldsAxes({}, {__yoptions: 'flip'}).y).toEqual({autorange: 'reversed', range: undefined}); + }); + test('zoomed range is kept when applying unrelated options', () => { + expect(evalChangesFromFieldsAxes({yaxis: ZOOMED}, {__yoptions: ''}).y).toEqual({range: [3, 4]}); + }); + test('clearing bounds keeps the range they set (indistinguishable from a zoom)', () => { + const fromBounds = {range: [2, 8], autorange: false}; + expect(evalChangesFromFieldsAxes({yaxis: fromBounds}, {__yoptions: ''}).y).toEqual({range: [2, 8]}); + }); + }); + + describe('bounds', () => { + test('both bounds, other axis untouched', () => { + const {x, y} = evalChangesFromFieldsAxes({xaxis: FIXED, yaxis: FIXED}, {__yoptions: '', ...bounds(2, 8)}); + expect(y).toEqual({range: [2, 8], autorange: false}); + expect(x).toEqual({}); + }); + test('reverse with bounds (FIREFLY-2103)', () => { + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: 'flip', ...bounds(2, 8)}).y) + .toEqual({range: [8, 2], autorange: false}); + }); + test('un-reverse with bounds (FIREFLY-2103)', () => { + expect(evalChangesFromFieldsAxes({yaxis: REV}, {__yoptions: '', ...bounds(2, 8)}).y) + .toEqual({range: [2, 8], autorange: false}); + }); + test('missing bound is filled from the current range', () => { + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: '', ...bounds(2)}).y) + .toEqual({range: [2, 9], autorange: false}); + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: 'flip', ...bounds(undefined, 8)}).y) + .toEqual({range: [8, 1], autorange: false}); + }); + test('bounds are kept in fireflyLayout (used by heatmap binning and the dialog)', () => { + const {changes} = evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: '', ...bounds(2, 8)}); + expect(changes['fireflyLayout.yaxis.min']).toBe('2'); + expect(changes['fireflyLayout.yaxis.max']).toBe('8'); + }); + }); + + describe('axis column change (reset)', () => { + test('no bounds: range is dropped for autorange in the requested direction', () => { + expect(evalChangesFromFieldsAxes({yaxis: REV}, {__yoptions: '', __yreset: 'true'}).y) + .toEqual({autorange: true, range: undefined}); + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: 'flip', __yreset: 'true'}).y) + .toEqual({autorange: 'reversed', range: undefined}); + }); + test('bounds: direction comes from the reverse option, not the old range', () => { + expect(evalChangesFromFieldsAxes({yaxis: REV}, {__yoptions: '', __yreset: 'true', ...bounds(2, 8)}).y) + .toEqual({range: [2, 8], autorange: false}); + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: 'flip', __yreset: 'true', ...bounds(2, 8)}).y) + .toEqual({range: [8, 2], autorange: false}); + }); + test('a single bound with nothing to fill the other from, autoscales in the requested direction', () => { + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: '', __yreset: 'true', ...bounds(2, undefined)}).y) + .toEqual({autorange: true, range: undefined}); + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: 'flip', __yreset: 'true', ...bounds(undefined, 8)}).y) + .toEqual({autorange: 'reversed', range: undefined}); + }); + test('both axes reset together (histogram column change)', () => { + const {x, y} = evalChangesFromFieldsAxes({xaxis: FIXED, yaxis: REV}, + {__xoptions: '', __yoptions: '', __xreset: 'true', __yreset: 'true'}); + expect(x).toEqual({autorange: true, range: undefined}); + expect(y).toEqual({autorange: true, range: undefined}); + }); + }); + + describe('log scale', () => { + test('linear to log converts the current range to log10 units', () => { + const c = evalChangesFromFieldsAxes({yaxis: AUTO}, {__yoptions: 'log'}).y; + expect(c.type).toBe('log'); + expect(c.autorange).toBe(false); + expect(c.range[0]).toBeCloseTo(0); + expect(c.range[1]).toBeCloseTo(Math.log10(9)); + }); + test('log to linear converts the current range back to data units', () => { + const c = evalChangesFromFieldsAxes({yaxis: LOG}, {__yoptions: ''}).y; + expect(c.type).toBe('linear'); + expect(c.autorange).toBe(false); + expect(c.range[0]).toBeCloseTo(1); + expect(c.range[1]).toBeCloseTo(10); + }); + test('scale switch keeps a reversed axis reversed', () => { + const c = evalChangesFromFieldsAxes({yaxis: REV}, {__yoptions: 'flip,log'}).y; + expect(c.autorange).toBe(false); + expect(c.range[0]).toBeCloseTo(Math.log10(9)); + expect(c.range[1]).toBeCloseTo(0); + }); + test('log, reverse and bounds together', () => { + const c = evalChangesFromFieldsAxes({yaxis: FIXED}, {__yoptions: 'flip,log', ...bounds(2, 8)}).y; + expect(c.range[0]).toBeCloseTo(Math.log10(8)); + expect(c.range[1]).toBeCloseTo(Math.log10(2)); + }); + test('missing bound is filled from a log range in data units', () => { + const c = evalChangesFromFieldsAxes({yaxis: LOG}, {__yoptions: 'log', ...bounds(2)}).y; + expect(c.range[0]).toBeCloseTo(Math.log10(2)); + expect(c.range[1]).toBeCloseTo(1); + }); + test('staying on log keeps the range and leaves autorange alone', () => { + // used to rebuild the range and set autorange false on every Apply, freezing an autoscaled log axis + expect(evalChangesFromFieldsAxes({yaxis: LOG}, {__yoptions: 'log'}).y).toEqual({type: 'log', range: [0, 1]}); + }); + test('linear to log with a range reaching 0 or below falls back to autorange', () => { + // used to produce a NaN range + expect(evalChangesFromFieldsAxes({yaxis: {range: [-5, 9], autorange: true}}, {__yoptions: 'log'}).y) + .toEqual({type: 'log', autorange: true, range: undefined}); + expect(evalChangesFromFieldsAxes({yaxis: {range: [-5, 9], autorange: true}}, {__yoptions: 'log,flip'}).y) + .toEqual({type: 'log', autorange: 'reversed', range: undefined}); + }); + }); + + describe('fields without the axis options (e.g. spectrum unit conversion)', () => { + test('no axis range fields leave the range alone', () => { + expect(evalChangesFromFieldsAxes({yaxis: REV}, {'layout.yaxis.title.text': 'flux'}).y).toEqual({}); + }); + test('bounds without the reverse option keep the current direction', () => { + expect(evalChangesFromFieldsAxes({yaxis: REV}, bounds(2, 8)).y).toEqual({range: [8, 2], autorange: false}); + expect(evalChangesFromFieldsAxes({yaxis: FIXED}, bounds(2, 8)).y).toEqual({range: [2, 8], autorange: false}); + }); + }); + + test('x axis behaves the same', () => { + const c = evalChangesFromFieldsAxes({xaxis: FIXED}, + {__xoptions: 'flip', 'fireflyLayout.xaxis.min': '2', 'fireflyLayout.xaxis.max': '8'}).x; + expect(c).toEqual({range: [8, 2], autorange: false}); + }); +}); + +describe('evalAxisRangeChanges', () => { + test('reset without axis options or bounds leaves the axis alone', () => { + expect(evalAxisRangeChanges({a: 'y', fields: {__yreset: 'true'}, axisLayout: REV})).toEqual({}); + }); + test('axis options key present with no value means nothing checked', () => { + expect(evalAxisRangeChanges({a: 'y', fields: {__yoptions: undefined}, axisLayout: REV})) + .toEqual({'layout.yaxis.range': [1, 9], 'layout.yaxis.autorange': false}); + }); + test('bounds without axis options use the current scale', () => { + const c = evalAxisRangeChanges({a: 'y', fields: bounds(2, 8), axisLayout: LOG}); + expect(c['layout.yaxis.range'][0]).toBeCloseTo(Math.log10(2)); + expect(c['layout.yaxis.range'][1]).toBeCloseTo(Math.log10(8)); + }); + test('no axis layout at all', () => { + expect(evalAxisRangeChanges({a: 'y', fields: {__yoptions: 'flip'}})) + .toEqual({'layout.yaxis.autorange': 'reversed', 'layout.yaxis.range': undefined}); + }); +}); diff --git a/src/firefly/js/charts/ui/options/BasicOptions.jsx b/src/firefly/js/charts/ui/options/BasicOptions.jsx index ba8af7d738..2acd45fd8e 100644 --- a/src/firefly/js/charts/ui/options/BasicOptions.jsx +++ b/src/firefly/js/charts/ui/options/BasicOptions.jsx @@ -1,6 +1,6 @@ import React, {useCallback, useEffect} from 'react'; import PropTypes from 'prop-types'; -import {get, isArray, isEmpty, isString, isUndefined, memoize, reverse, set} from 'lodash'; +import {get, isArray, isEmpty, isString, isUndefined, set} from 'lodash'; import {DEFAULT_PLOT2D_VIEWER_ID} from '../../../visualize/VisConst'; import {dispatchChartAdd, dispatchChartUpdate, getChartData} from '../../ChartsCntlr.js'; @@ -24,6 +24,7 @@ import {hideColSelectPopup} from '../ColSelectView.jsx'; import {addColorbarChanges} from '../../dataTypes/FireflyHeatmap.js'; import {colorsOnTypes, getChartProps, toRGBA, TRACE_COLORS, uniqueChartId, isSpectrum} from '../../ChartUtil.js'; import {colorscaleNameToVal} from '../../Colorscale.js'; +import {evalAxisRangeChanges, isAxisReversed} from '../../AxisRangeUtil.js'; import MAGNIFYING_GLASS from 'html/images/icons-2014/magnifyingGlass.png'; import {ToolbarButton} from '../../../ui/ToolbarButton.jsx'; @@ -55,10 +56,7 @@ function getOptions(a, layout) { if ( (isUndefined(showgrid) && get(layout, `${a}axis.gridwidth`)) || showgrid) { opts.push('grid'); } - const range = get(layout, `${a}axis.range`) || []; - const autorange = get(layout, `${a}axis.autorange`); - const reversed = (autorange === 'reversed') || (range[1] < range[0]); - if (reversed) { + if (isAxisReversed(get(layout, `${a}axis`))) { opts.push('flip'); } if (get(layout, `${a}axis.side`) === (a==='x'?'top':'right')) { @@ -320,30 +318,6 @@ export function evalChangesFromFields(chartId, tbl_id, fields) { ['x','y'].forEach((a) => { if (k === `__${a}options`) { const opts = v || ''; - - // helper hidden fields __xreset and __yreset keep track of whether x and y have changed - // if this field is set, we'd like to clear range - const range = !fields[`__${a}reset`] && get(layout, `${a}axis.range`); - - if (opts.includes('flip')) { - if (range) { - // reverse mutates the original array - changes[`layout.${a}axis.range`] = (range[0] { - let minUser = parseFloat(get(changes, `fireflyLayout.${a}axis.min`)); - let maxUser = parseFloat(get(changes, `fireflyLayout.${a}axis.max`)); - - if (!Number.isNaN(minUser) || !Number.isNaN(maxUser) || changes[`layout.${a}axis.type`]) { - // range values of a log axis are logs - convert them back - const range = changes[`layout.${a}axis.range`] && - (get(layout, `${a}axis.range`, []).map(get(layout, `${a}axis.type`) === 'log' ? (e)=>Math.pow(10, e) : (e)=>e)); - if (Number.isNaN(minUser) && range) { - minUser = Math.min(range[0], range[1]); - } - if (Number.isNaN(maxUser) && range) { - maxUser = Math.max(range[0], range[1]); - } - - if (!Number.isNaN(minUser) || !Number.isNaN(maxUser)) { - const autorange = changes[`layout.${a}axis.autorange`]; - const reversed = (autorange === 'reversed') || (!autorange && range[1] < range[0]); - changes[`layout.${a}axis.range`] = getRange(minUser, maxUser, changes[`layout.${a}axis.type`] === 'log', reversed); - changes[`layout.${a}axis.autorange`] = false; - } - } - }); -} - - -/** - * Get range for a plotly axis - * Plotly requires range to be reversed if the axis is reversed, - * and limits to be log if axis scale is log - * @param min - minimum value - * @param max - maximum value - * @param isLog - true, if an axis uses log scale - * @param isReversed - true, if the axis should be reversed - * @returns {Array} an array for axis range property in plotly layout - */ -function getRange(min, max, isLog, isReversed) { - const [r1, r2] = isReversed ? [max, min] : [min, max]; - return isLog ? [Math.log10(r1), Math.log10(r2)] : [r1, r2]; + return { + ...changes, + ...evalAxisRangeChanges({a: 'x', fields, axisLayout: layout.xaxis}), + ...evalAxisRangeChanges({a: 'y', fields, axisLayout: layout.yaxis}), + }; } /**