Skip to content

Conversation

@t-b
Copy link
Collaborator

@t-b t-b commented Jul 8, 2024

Implement a MVP

  • Don't support vs
  • AppendToTable with with
  • Multiple tables with and
table(0...10)

gives

image

@t-b t-b self-assigned this Jul 8, 2024
@t-b t-b removed their assignment Feb 21, 2025
@timjarsky timjarsky requested a review from Copilot August 7, 2025 17:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a minimum viable product (MVP) for the table operation in SweepFormula, allowing data to be displayed in a table format instead of the default graph view.

  • Adds a new table operation that can be applied to datasets
  • Modifies the plotting logic to display data as an editable table when the table operation is used
  • Implements basic table functionality without support for advanced features like vs, with, or and operations

@MichaelHuth MichaelHuth self-assigned this Nov 7, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch 2 times, most recently from 90e95b9 to 529c345 Compare November 8, 2025 22:50
@MichaelHuth
Copy link
Collaborator

@timjarsky Here is a first version for playing around.

If you enclose an expression in table(expr) then it is presented in a table. table must be the outermost operation to have an effect. (e.g. table(1) + 1 does not result in a table because the + is the outermost operation applied)

The table(s) are in a separate panel.
Combinations with and and with are supported. Any x-wave provided with vs is ignored when the y-wave is targeted for a table. table operation on the x-wave has no effect.

f.e.

1
and
table(2)
and
3
and
table(4)

results in two panels with two sub windows each, one with two tables and the other with two plots.

table(1)
with
table(2)

One subwindow with one table and two columns. Expressions added with with are added as new columns.

Also multiple datasets from a single expression result in new columns per dataset.

Special case mixing:

table(1)
with
2

Is split into a table and a plot.

If you use only tables or only plots the second window is not presented as it would contain no data.

SF_DM_NORMAL display mode is supported as well.

@MichaelHuth MichaelHuth assigned timjarsky and unassigned MichaelHuth Nov 8, 2025
@t-b
Copy link
Collaborator Author

t-b commented Nov 10, 2025

@MichaelHuth The runners work again. The failure in https://github.com/AllenInstitute/MIES/actions/runs/19199707109/job/55010723400 is our documentation check.

@timjarsky
Copy link
Collaborator

@MichaelHuth This is working well. Please add a right-click menu option to bring the databrowser to the front, similar to what we do for the plots. Also, could you make the dimension labels on the tables more informative? Thank you.

@timjarsky timjarsky assigned MichaelHuth and unassigned timjarsky Nov 12, 2025
@t-b
Copy link
Collaborator Author

t-b commented Nov 12, 2025

The table(s) are in a separate panel.

Why in a separate panel and not as a subwindow in the existing one?

@MichaelHuth
Copy link
Collaborator

The table(s) are in a separate panel.

Why in a separate panel and not as a subwindow in the existing one?

In the current panel as a subwindow the resulting size of the table might be two small and it could be inconvenient to use without additional elements to change the guide to adjust subwindow distribution.

Also the complexity stays similar as a user can have a formula like:

table(1)
with
2

then this results in a plot and a table. In a single panel I would also need to reserve subwindows for each display type and remove empty ones afterwards.

If you think that moving different display types to a new panel is too inflationary regarding the number windows I can wrap everything in the same panel.

@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch from 529c345 to 40341b0 Compare November 14, 2025 14:33
@MichaelHuth
Copy link
Collaborator

@timjarsky

Also, could you make the dimension labels on the tables more informative? Thank you.

I changed the table that it shows dimension labels. The dimension labels are from the data waves that are from the result in the expression in table(expr). This means that depending on that operation the resulting wave can have labels or not.
e.g. for the result from select() we add dimension labels, but most operations do not add dimension labels.
So tables are rather limited in that respect.

In Igor 10 for tables also tooltips work. I can retrieve the indices and the wave where the cursor is currently positioned. So it would be possible to generate some tooltip text including information present in the JSON wave note.
Would such a tooltip a viable option?

@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch from 40341b0 to 832c2b7 Compare November 14, 2025 15:43
@t-b
Copy link
Collaborator Author

t-b commented Nov 14, 2025

In Igor 10 for tables also tooltips work. I can retrieve the indices and the wave where the cursor is currently positioned. So it would be possible to generate some tooltip text including information present in the JSON wave note.
Would such a tooltip a viable option?

Can the tooltip show the formula which created the table column? If yes I'd say it's worth the effort.

@t-b
Copy link
Collaborator Author

t-b commented Nov 17, 2025

@MichaelHuth Chatted with Tim about this today. And we need to add a way for the user to C&P the sweepformula code which produced the table. So maybe add another column with just the SF code which produced the other columns?

@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch from 832c2b7 to 7289397 Compare November 18, 2025 00:06
@MichaelHuth
Copy link
Collaborator

@MichaelHuth Chatted with Tim about this today. And we need to add a way for the user to C&P the sweepformula code which produced the table. So maybe add another column with just the SF code which produced the other columns?

We can collect the formulas that lead to a certain table window in the plotter and put this in the user data of the window. I am not so convinced that adding another column is a good solution as it contains then data that is not part of the SF output.
If the user does not need to see the formulas (or the tooltip is enough for this) then I would prefer a button (like here on github) that just puts the formulas in the clipboard.

Should this output then be just a list of formulas or should it be adapted with "with" inserted?

Variant 1:

formula(a)
formula(b)

Variant 2:

formula(a)
with
formula(b)

The latter would have the advantage that it can be pasted as is to a SF notebook again.

@timjarsky Would a button with the "put in clipboard" functionality work as well?

@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch 2 times, most recently from 7e394f4 to 41684b4 Compare November 19, 2025 15:34
@MichaelHuth
Copy link
Collaborator

@timjarsky I added a context menu entry that allows to copy the formulas that were evaluated to create the table to the clipboard. If multiple formulas are part of the table then the with keyword is inserted, such that this can easily pasted to another sweepformula notebook. You have to have the table subwindow the active window before right clicking, so it might be necessary to left click on the table first.

Also check if the tooltip is ok as it is or if it needs some more information.

@MichaelHuth MichaelHuth assigned timjarsky and unassigned MichaelHuth Nov 19, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch 2 times, most recently from b7e25fe to 06aec05 Compare November 21, 2025 11:55
timjarsky
timjarsky previously approved these changes Nov 24, 2025
@timjarsky timjarsky assigned MichaelHuth and unassigned timjarsky Nov 24, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch from 06aec05 to d8fcbc2 Compare November 25, 2025 16:37
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Nov 25, 2025
@t-b
Copy link
Collaborator Author

t-b commented Nov 26, 2025

Playing around

  • unexpected looking empty plot
sel = select(selsweeps(-1)) # note the -1 to create an empty plot
dat = data($sel)

$dat

and

table(1)

creates the following plot

empty-plot
  • no cleanup on error
error-cleanup
  • What does
table(data()) vs data()

do? Here it looks like that the vs part is ignored.

The documentation says:

X-waves specified by the vs keyword are not appended to the table.

Maybe this needs to also say that the vs part is ignored?

Review:

b8d1a0c (GUI util: Add utility function to remove all columns from a table, 2025-11-13)

Code and tests look good.

The commit message has

Function:
RemoveAllColumnsFromTable

which looks like an left over. If you want to have the function name in the commit message I would rephrase
the subject to something like

GUI util: Add RemoveAllColumnsFromTable

7537f7e (Utils: Add function to test if a wave has dimension labels, 2025-11-18)

Same comment about commit message.

Code and test looks good.

2b5bf7a (SF: Add support to plotter to display data in tables, 2025-11-13)

SF_TileExistingData:

  • Either use our usual naming for for loop indizes with i being the most outer, j more innner etc. or use
    totally different names. But i for inner is not good.

  • I don't see a reason to have guideName1 and guideName2 instead of only guideName.

  • win = RemoveEnding(win, "#" + StringFromList(ItemsInList(win, "#") - 1, win, "#")) should use
    LastStringFromList to give win = RemoveEnding(win, "#" + LastStringFromList(win, "#"))

95f877c (SF: Add operation table, 2025-11-14)

Please prefer SFH_GetArgumentAsWave. Nice tests!

42e7286 (SF: Add tooltip information to table display, 2025-11-18)

Tooltip looks great. Please use key = RemovePrefix(prop, "/") instead of key = prop[1, inf] as the former
is more readable and safer (and yes also slower, but I think we can live with that).

ff1025a (SF: Use only label display in table view if the wave actually has labels, 2025-11-18)

Good.

1bf9405 (SF: In plotter add formulas for tables to userdata of window, 2025-11-18)

Makes sense.

64ee6ae (SF: Add context menu option on SF table to copy formulas to clipboard, 2025-11-19)

Yuup.

b1967e6 (SF: Workaround for Igor not changing subwindow focus on right click, 2025-11-21)

commit message: doe snot -> does not

d8fcbc2 (SF: Add documentation for table operation to rst and ifn, 2025-11-19)

Good, but see my earlier comment about vs.

@t-b t-b assigned MichaelHuth and unassigned t-b Nov 26, 2025
@t-b
Copy link
Collaborator Author

t-b commented Nov 26, 2025

As this is my PR I can't request changes.

@t-b t-b marked this pull request as ready for review November 26, 2025 19:48
@t-b t-b requested a review from MichaelHuth as a code owner November 26, 2025 19:48
@MichaelHuth
Copy link
Collaborator

The documentation says:

X-waves specified by the vs keyword are not appended to the table.

Maybe this needs to also say that the vs part is ignored?

The part after vs is evaluated and only the result is not displayed in the plotter. So technically it is only ignored regarding to plotting, but not for evaluation. So e.g. store(xxx) would still be executed.

I can of course extend the documentation to be more precise for this.

A utility function to remove all columns from a table

This is the equivalent for RemoveTracesFromGraph for plots with the
base functionality.
A function to test if a wave has dimension labels

The function checks by default only the first dimindex because waves with
labels typically start with the first element labelled.
There is an optional argument though to increase the lookup depth.
Tables introduce a second type of displaying data (after plots) to the plotter.
Tables are displayed in a separate panel.

Therefore in SF_PreparePlotter the plotGraphs wave that lists all window names
is extended with a new row for the names of the table subwindows.
The creation of plotGraphs is moved to an own getter function.

The panel or window creation for SF_DM_NORMAL mode is moved to an own function
SF_NewSweepFormulaBaseWindow

For the equivalent of RemoveTracesFromGraph the function RemoveAllColumnsFromTable
is used for tables.

Since not all SubWindows prepared by SF_PreparePlotter might be used in plotting
the subWindows (SF_DM_SUBWINDOWS mode) are tiled only between the windows that
actually contain data after all data was plotted in SF_TileExistingData.
operation tags the output wave to hint the plotter that the data should be displayed
in a table. The operation does not change the data otherwise.

table(expression)
The tooltip displays a selection of properties of the data when available
The properties can be set in SF_GetTableTooltipProperties

The tooltip function attempts automatically to read it as string or number.

Adapted plotting tests for the change that the wave in the plotter contains
now always the formula as JSON wave note.
(The direct result from the executor does not)
The formulas are copied with the "with" keyword if the table consists of multiple
formulas.
There is a bug, where Igor does not change the focus to a subwindow when
right clicked. This prevents that the clicked window name can be retrieved
on the basis of the currently active window.

WM suggested a workaround where the last right clicked window is stored in
a window hook and then used in the menu handler.

This commit implements this workaround.
@t-b
Copy link
Collaborator Author

t-b commented Nov 27, 2025

I can of course extend the documentation to be more precise for this.

That would be good.

@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch from d8fcbc2 to 14a8bcf Compare November 27, 2025 13:30
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Nov 27, 2025
@t-b
Copy link
Collaborator Author

t-b commented Nov 27, 2025

@MichaelHuth Thanks for reassigning me. The CI looks broken, I'll fix it once the VPN works again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants