Skip to content

Conversation

@MichaelHuth
Copy link
Collaborator

Part of #2581

@MichaelHuth MichaelHuth self-assigned this Dec 3, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch 2 times, most recently from 56ba8c5 to aee3efd Compare December 3, 2025 14:00
@MichaelHuth MichaelHuth force-pushed the feature/2586-refactor_sf_plotter branch from 1cc4c38 to 00e5efa Compare December 3, 2025 14:02
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Dec 3, 2025
@t-b t-b force-pushed the feature/2171-add-table-support-in-sweepformula branch from aee3efd to 4915ea5 Compare December 3, 2025 18:19
@MichaelHuth MichaelHuth force-pushed the feature/2171-add-table-support-in-sweepformula branch from 4915ea5 to 1c6f1c4 Compare December 4, 2025 03:50
Base automatically changed from feature/2171-add-table-support-in-sweepformula to main December 4, 2025 07:22
t-b
t-b previously approved these changes Dec 4, 2025
Copy link
Collaborator

@t-b t-b left a comment

Choose a reason for hiding this comment

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

New code LGTM. Needs conflict resolution though.

@t-b t-b assigned MichaelHuth and unassigned t-b Dec 4, 2025
Copilot AI review requested due to automatic review settings December 4, 2025 13:24
@MichaelHuth MichaelHuth force-pushed the feature/2586-refactor_sf_plotter branch from 00e5efa to aa4c8ac Compare December 4, 2025 13:24
@MichaelHuth
Copy link
Collaborator Author

@t-b I moved the remaining changes from #2171 here.

@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Dec 4, 2025
Copilot finished reviewing on behalf of MichaelHuth December 4, 2025 13:27
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 refactors the SweepFormula plotter to separate formula evaluation from plotting logic, improving code organization and maintainability. The change replaces a struct-based approach for plot metadata with a wave-based approach, enabling better separation of concerns between evaluation and display phases.

Key Changes:

  • Extracted formula evaluation into new function SF_PlotterGetDataFromFormulas() that pre-evaluates all formulas before plotting
  • Converted SF_PlotMetaData struct to text wave with dimension labels for plot metadata storage
  • Updated error handling to collect errors during evaluation and abort at the end if any occurred

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
Packages/MIES/MIES_Structures.ipf Removed obsolete SF_PlotMetaData structure definition
Packages/MIES/MIES_WaveDataFolderGetters.ipf Added wave getter functions for SF plot metadata and data organization
Packages/MIES/MIES_SweepFormula.ipf Refactored plotter with new evaluation function, converted struct access to wave indexing, improved error handling, fixed typos
Packages/MIES/MIES_SweepFormula_PSX.ipf Updated function signatures to use wave-based metadata instead of struct
Packages/MIES/MIES_Utilities_WaveHandling.ipf Fixed comment typo, added safety check for deep parameter
Packages/MIES/MIES_GuiUtilities.ipf Added null check and non-existent window handling to GetAllWindows function
Packages/tests/Basic/UTF_GuiUtilities.ipf Added comprehensive tests for GetAllWindows function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Formula evaluation was splitted from plotting and is now done in a separate function
before the plotting loop is run. The formula and meta data results are gathered in that
function.

The structure PlotMetaData was removed as it can not be saved in an indexable wave.
It was replaced by a text wave.

The function that evaluated the formulas catches evaluation errors and returns an error code.
If this error code is set, then SFH_ASSERT has stored error related information.
However, we want to display already formulas that are correct in plots.
The evaluation does not add results for formulas that errored out.
Thus, the plotting loop runs, but does not include results where the formula evaluation had an error.
After the loop ran, an Abort is triggered if there was an evaluation error such that the outer catch
can handle error output and notebook modification.

This also allows the plotter loop to use SFH_ASSERT.

The behavior change is that only the last evaluation error is shown, whereas before the first
evaluation error was shown.
@MichaelHuth MichaelHuth force-pushed the feature/2586-refactor_sf_plotter branch from aa4c8ac to 767c569 Compare December 4, 2025 13:58
@t-b t-b removed their assignment Dec 4, 2025
@t-b
Copy link
Collaborator

t-b commented Dec 4, 2025

Failing CI.

@MichaelHuth
Copy link
Collaborator Author

The reason for the fail is:
The test case calls

psx(...)
and
psxstats(...)

where in main psx(...) gets evaluated then PSX_Plot (and PSX_PostPlot) is called that setups the plot, then psxstats is evaluated, where the evaluation relies on an existing psx plot.

With the changed the evaluation is done first, such that psx(...) is evaluated, then psxstats(...) and that fails because there is no plot window yet.

PSX_Plot / PSX_PostPlot can be called later but psxstats evaluation requires results from these calls. I do not see a simple way to split the data preparation and plotting in PSX.

I think this prevents generally to split off the evaluation now from displaying the data in the plotter. Considering that this should prepare the plotter for operations with plotting "instructions", the way would be to incrementally build up the output panel with subsequently adding plot subwindows.

@t-b
Copy link
Collaborator

t-b commented Dec 5, 2025

Thanks for digging. What would then be the way forward?

@t-b
Copy link
Collaborator

t-b commented Dec 5, 2025

From the top of my head. We could delay running psxstats after the plotting.

@MichaelHuth
Copy link
Collaborator Author

Delaying the plotting of psxstats would not help, because already the evaluation of a formula that contains psxstats requires that any previously evaluated formula containing psx was already plotted. For the general case the plotter does not know if psxstats was one operation that was part of the evaluation.

I first thought I move the storage for the psx working DF from the plot window to the data browser window (as far as I tested multiple psx plots separated by and do not work anyway). Then I the data creation part would need to be done in PSX_Operation already, such that it is already available when psxstats is evaluated. But I think psxstats also interacts with elements of the psx plot, which would not be present at that time...

So instead of trying to make PSX work with splitting evaluation and plotting I now think that currently a feasible way is to change the plotter that it also creates the plot/table subwindows dynamically. Such that there is always an evaluation step and a plotting step (if the evaluation yielded results). The plotting adds then a new subwindow and everything is tiled at the end.
If the evaluation returns data from an operation with full plotting information (like a ivscc_apfreqency should be) then more than one subwindow for this evaluation is added.

Logically this means that I pull the window / guide creation and plotGraphs wave filling in the loop that parses the formulas.

@t-b
Copy link
Collaborator

t-b commented Dec 7, 2025

So instead of trying to make PSX work with splitting evaluation and plotting I now think that currently a feasible way is to change the plotter that it also creates the plot/table subwindows dynamically. Such that there is always an evaluation step and a plotting step (if the evaluation yielded results). The plotting adds then a new subwindow and everything is tiled at the end.

Sounds good!

@MichaelHuth
Copy link
Collaborator Author

This PR is replaced by #2592

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants