FIREFLY-2006: Charts Grouping functionality by kpuriIpac · Pull Request #1992 · Caltech-IPAC/firefly

kpuriIpac · 2026-08-11T21:34:43Z

Ticket: FIREFLY-2006

Added grouping functionality to scatter plots (to allow user to group by any enum like columns)
Generalizing grouping to all scatter plots (we previously already supported grouping by order column for spectra) exposed several existing chart bugs (some related to grouping, some to filtering/selection, etc.).
- This PR fixes those bugs as well.
makeScatterGroupByChanges is the main function handling grouping logic (in ChartUtil). Some other supporting functions are also in ChartUtil. My reasoning behind keeping these here was that even though we only offer grouping for scatter plots, it's general enough to belong in ChartUtil.
- open to feedback and moving this elsewhere, perhaps ScatterOptions.
Final note: I tried to keep code separated (following separation of concern logic) & generic, as much as possible. And tightened up some areas prone to frequent errors. But an overall refactor of Firefly's Charts (in the near future, perhaps last cycle of 2026) will make working with charts much easier moving forward.

Testing:

IRSAViewer, Spherex, Euclid, Firefly
Do searches on any of the apps for regular scatter plots or spectra. Play around with the chart dialog.
- In the chart dialog, Spectra should load with the chart dialog already set to Group by: Order, and all other scatter plots should load with Group by: None
- Apply Group By with any column. Select any trace and try changing trace style/symbol, etc. Set back Group By to None.
- While trying these, make a selection on a portion of the chart, and from the toolbar, try both select the enclosed points, and Filter on the selected points. And then remove the selection/filter. Do this after you set Group By to a column and also after you set it back to None.
The above workflow led to several bugs I encountered and fixed, ensure you don't run into any more.
- After selection/filtering - also take a look at the Trace Options and Chart Options in the charts dialog. To make sure none of your selections changes (filtering would sometimes remove reverse from X/Y axes, even on ops - under Chart Options, as an example of a bug).
Also, for spectra, try turning on/off Spectral Lines from the chart dialog as well. And change traces while Spectral Lines are on. This should all work without any bugs.

kpuriIpac · 2026-08-15T01:56:31Z

@robyww @jaladh-singhal

Updates, PR updated as of 8/14, bug free from all my tesing:

Lots of changes initially came from bugs discovered, but the code attempts to tighten the logic and improve existing code as well (as much as possible, before our big chats refactor in the near future)
Feel free to ask me about any of the logic - I’ve tried describing some of it via in code comments
Test builds now updated
- Test chart dialog as you please. Overplot all kinds of charts (or add different kinds of charts). Remove active traces (of the original chart or the one you overplotted)
  - Filter/select points where applicable
  - Group by should only show for scatter plots, including spectrum plots
  - Ungroup (Group by: None) / Group By and play around with chart toolbar as well

Please report any new bugs, style fix suggestions, etc.

robyww

This is looking really good! I think it will be a very nice feature.

Code

Several small things to clean up.
Glad you added stuff to ChartUtil, I have a concern there is a lot of redundancy in the chart code.

Testing and UI.

Test when very well. I did not run into any issues.
I looks and works very nice! Easy to use!
UI change? Maybe in the grouping case we do not need the trace selection on the toolbar. I don't really see what it does.

robyww · 2026-08-18T16:58:50Z

    fields && Object.entries(fields).forEach( ([k,v]) => {
+        // do not let an empty histogram label erase the shared X-axis label while overplotting
+        if (k === 'layout.xaxis.title.text' &&
+            (!v || v === 'undefined') &&


(!v || v === 'undefined')

I don't understand this line. can it just be !v. Am I missing something?

robyww · 2026-08-18T17:05:02Z

+
+    // The field can be undefined for one render while the dialog mounts
+    // Use live form state when present, otherwise fall back to chart state
+    const isEnabledField = useStoreConnector(() => getFieldVal(groupKey, ENABLED_KEY), [groupKey]);


just use, you don't need groupKey it is picked up from the context.

const isEnabledField- useFieldValueOnly(ENABLED_KEY)

robyww · 2026-08-18T17:06:15Z


    // TODO: combine different source tables to a client-side table
-    const sourceOptions = useFieldValueOnly(SOURCE_OPTIONS_KEY, initialSourceOptions);
+    const sourceOptions = useStoreConnector(() => getFieldVal(groupKey, SOURCE_OPTIONS_KEY), [groupKey]) ?? initialSourceOptions;


same here but also takes default value

const sourceOptions = useFieldValueOnly(SOURCE_OPTIONS_KEY, initialSourceOptions)

robyww · 2026-08-18T17:10:10Z

+        redshift: fields[sfFieldKeys.redshift] ?? currentSFOptionFields.redshift,
+        userSpecified: fields[sfFieldKeys.userSpecified] ?? currentSFOptionFields.userSpecified
+    };
+    const xUnitChanged = xUnit !== undefined && xUnit !== fireflyData?.[activeTrace]?.xUnit;


use isDefined(xUnit) (in WebUtil.js) it is easier to read

robyww · 2026-08-18T17:24:56Z

-            const {tbl_id} = tablesources[activeDataTrace] || {};
-            if (!tbl_id) return;
+            const tbl_id = getTblIdFromChart(chartId, activeDataTrace);
+            if (!tbl_id || !getTblById(tbl_id)) return;


I think you can just say getTblById(tbl_id) if not modify getTblById to have a

if (!tbl_id) return;

robyww · 2026-08-18T17:54:42Z

-    const chartData = cloneDeep(omit(getChartData(chartId), ['_original', 'mounted']));
-    chartData?.tablesources?.forEach((ts) => Reflect.deleteProperty(ts, '_cancel'));
+    const chartData = cloneDeep(omit(getChartData(chartId), ['_original', 'mounted', 'selection']));
+    if (chartData?.layout) delete chartData.layout.selections;


if you are going to use Reflect.deleteProperty you should do it here as well.

robyww · 2026-08-18T18:08:36Z

    };

-    return (chartIds?.length > 1)
+    const unresolvedChartIds = chartIds?.[0] instanceof Promise;


I have read doing an instanceof Promise was not very reliable. I look it up on AI and it pointed out some issues.

Why don't you make a isPromsie in WebUtil.js that looks something like

const isPromise = (p) => isFunction(p?.then);

line 245 is the other place it is use.
In out code base we only use instanceof Promise in this file.

robyww · 2026-08-18T18:10:26Z

    };

-    return (chartIds?.length > 1)
+    const unresolvedChartIds = chartIds?.[0] instanceof Promise;


I have read doing an instanceof Promise was not very reliable. I look it up on AI and it pointed out some issues.

Why don't you make a isPromsie in WebUtil.js that looks something like

const isPromise = (p) => isFunction(p?.then);

line 245 is the other place it is use.
In out code base we only use instanceof Promise in this file.

robyww · 2026-08-18T18:15:29Z

+        ...(traceTS.options || {}),
+        ...(fireflyData?.[traceNum]?.options || {})


using ... is safe on undefined object so you don't need || {}
example

const a={}; const b=undefined; const c= {...a,...b};

robyww · 2026-08-18T18:19:04Z

@@ -212,7 +230,7 @@ export function addScatterChanges({changes, chartId, traceNum, tablesource, tabl

    const colors = get(changes, [`data.${traceNum}.marker.color`]);
    let cTipLabel = isArray(colors) ? get(mappings, 'marker.color') : '';


take out the get in the above two lines.

kpuriIpac · 2026-08-19T01:17:59Z

@robyww Thanks! I will address your feedback. Meanwhile, I found that when opening the chart dialog, the X and Y input lines would sometimes appear a split second late (moving in the UI, making for bad UX). Could be even more delayed for bigger tables/charts (since I changed how we rely on colValStats, and these inputs wait for colValStats to render). I fixed this in the latest commit. Feel free to do some basic testing again.

kpuriIpac added this to the 2026.3 milestone Aug 11, 2026

kpuriIpac self-assigned this Aug 11, 2026

kpuriIpac added enhancement Charts Anything related to charts labels Aug 11, 2026

kpuriIpac requested review from jaladh-singhal, loitly and robyww August 11, 2026 22:27

kpuriIpac force-pushed the FIREFLY-2006-charts-groups branch from c480db1 to 6a601e7 Compare August 15, 2026 00:56

robyww approved these changes Aug 18, 2026

View reviewed changes

kpuriIpac added 11 commits August 18, 2026 18:12

charts grouping option

9a6e9b2

fix several regression and new bugs

6389c93

try new cleaner refactor approach

a3835f7

fixed more bugs

378eb85

more fixes

0d82f3c

fixes continued

9fd1a41

fix axes bug after filtering

bfdf4a7

overplot bug fix

4ecbaf3

cleanup

93e982d

several robust fixes

5b1db6b

fix x-y col line delay opening dialog

6c9ad64

kpuriIpac force-pushed the FIREFLY-2006-charts-groups branch from 6a601e7 to 6c9ad64 Compare August 19, 2026 01:13

Navigation Menu

Uh oh!

FIREFLY-2006: Charts Grouping functionality - #1992

FIREFLY-2006: Charts Grouping functionality #1992
kpuriIpac wants to merge 11 commits into
devfrom
FIREFLY-2006-charts-groups

kpuriIpac commented Aug 11, 2026 •

edited

Loading

Uh oh!

kpuriIpac commented Aug 15, 2026

Uh oh!

robyww left a comment

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

robyww Aug 18, 2026

Uh oh!

kpuriIpac commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		...(traceTS.options \|\| {}),
		...(fireflyData?.[traceNum]?.options \|\| {})

		@@ -212,7 +230,7 @@ export function addScatterChanges({changes, chartId, traceNum, tablesource, tabl

		const colors = get(changes, [`data.${traceNum}.marker.color`]);
		let cTipLabel = isArray(colors) ? get(mappings, 'marker.color') : '';

Uh oh!

Conversation

kpuriIpac commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpuriIpac commented Aug 15, 2026

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Code

Testing and UI.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpuriIpac commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kpuriIpac commented Aug 11, 2026 •

edited

Loading