MIES icon indicating copy to clipboard operation
MIES copied to clipboard

Add table operation in SweepFormula

Open t-b opened this issue 1 year ago • 16 comments

Implement a MVP

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

gives

image

t-b avatar Jul 08 '24 20:07 t-b

@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 avatar Nov 08 '25 23:11 MichaelHuth

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

t-b avatar Nov 10 '25 19:11 t-b

@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 avatar Nov 12 '25 17:11 timjarsky

The table(s) are in a separate panel.

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

t-b avatar Nov 12 '25 23:11 t-b

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 avatar Nov 13 '25 03:11 MichaelHuth

@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 avatar Nov 14 '25 15:11 MichaelHuth

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 avatar Nov 14 '25 15:11 t-b

@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?

t-b avatar Nov 17 '25 20:11 t-b

@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 avatar Nov 18 '25 00:11 MichaelHuth

@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 avatar Nov 19 '25 16:11 MichaelHuth

@timjarsky I reported a bug to WM regarding the focussing of the subwindows when right clicking. They suggested a workaround for it and I added that here. This means that with this change it is not required now to give focus to a table subwindow before right clicking to copy the formulas to the clipboard. It will also work if the table subwindow below the mouse is not the active subwindow.

MichaelHuth avatar Nov 21 '25 11:11 MichaelHuth

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:

b8d1a0c495 (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

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

Same comment about commit message.

Code and test looks good.

2b5bf7ad4d (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, "#"))

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

Please prefer SFH_GetArgumentAsWave. Nice tests!

42e7286ca2 (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).

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

Good.

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

Makes sense.

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

Yuup.

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

commit message: doe snot -> does not

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

Good, but see my earlier comment about vs.

t-b avatar Nov 26 '25 19:11 t-b

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

t-b avatar Nov 26 '25 19:11 t-b

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.

MichaelHuth avatar Nov 27 '25 11:11 MichaelHuth

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

That would be good.

t-b avatar Nov 27 '25 13:11 t-b

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

t-b avatar Nov 27 '25 17:11 t-b

LGTM.

t-b avatar Dec 01 '25 21:12 t-b

@MichaelHuth One of the tests fails

•runwithOpts(testcase = "TestPlotting")
  Start of test "MIES with Basic"
  Entering test suite "UTF_SweepFormula.ipf"
  Entering test case "UTF_SweepFormula#TestPlotting"
  MIES BUG_TS: Encountered pending RTE: 725, ChildWindowList;Expected window name.
  BUG_TS: Should never be called during automated testing.
  MIES BUG_TS: Encountered pending RTE: 725, ChildWindowList;Expected window name.
  BUG_TS: Should never be called during automated testing.
  MIES BUG_TS: Encountered pending RTE: 725, ChildWindowList;Expected window name.
  BUG_TS: Should never be called during automated testing.
  MIES BUG_TS: Encountered pending RTE: 725, ChildWindowList;Expected window name.
  BUG_TS: Should never be called during automated testing.
  MIES BUG_TS: Encountered pending RTE: 725, ChildWindowList;Expected window name.
  BUG_TS: Should never be called during automated testing.
  MIES BUG_TS: Encountered pending RTE: 725, ChildWindowList;Expected window name.
  BUG_TS: Should never be called during automated testing.
  6 == 0: is false. Assertion "CHECK_EQUAL_VAR(bugCount_ts, 0)" failed in TEST_CASE_END_OVERRIDE➔TestHelperFunctions#CheckForBugMessages (UTF_HelperFunctions.ipf, line 24➔1069)
  Leaving test case "UTF_SweepFormula#TestPlotting"
  Failed with 1 errors
  Leaving test suite "UTF_SweepFormula.ipf"
  Test finished with 1 errors
    ▶ Assertion "CHECK_EQUAL_VAR(bugCount_ts, 0)" failed in TEST_CASE_END_OVERRIDE➔TestHelperFunctions#CheckForBugMessages (UTF_HelperFunctions.ipf, line 24➔1069)
  End of test "MIES with Basic"

t-b avatar Dec 02 '25 20:12 t-b

@t-b I added a fix for this.

MichaelHuth avatar Dec 03 '25 11:12 MichaelHuth

And this needs now conflict resolution. Sorry for that.

t-b avatar Dec 03 '25 13:12 t-b

Rebased to have removed IP10 CI.

t-b avatar Dec 03 '25 18:12 t-b

@MichaelHuth And another CI error:

  Test finished with 1 errors
  
    ? Uncaught runtime error 99:"GetUserData;expected name of a target window" in test case "UTF_SweepFormula_PSX#NoEventsAtAll" (UTF_SweepFormula_PSX.ipf)
  

t-b avatar Dec 03 '25 18:12 t-b

Thanks. LGTM.

t-b avatar Dec 04 '25 07:12 t-b