Skip to content

Add preference to use the 'optimized with last sample' AA postprocessor - #3915

Open
rjwills28 wants to merge 3 commits into
ControlSystemStudio:masterfrom
rjwills28:databrowser_optimLastSample_postprocessor
Open

Add preference to use the 'optimized with last sample' AA postprocessor#3915
rjwills28 wants to merge 3 commits into
ControlSystemStudio:masterfrom
rjwills28:databrowser_optimLastSample_postprocessor

Conversation

@rjwills28

Copy link
Copy Markdown
Contributor

A while back we introduced a new post processor to the Archive Appliance that better represents the data called 'Optimized with last sample'.

The history and detailed discussion of why this pp was required is here: ControlSystemStudio/cs-studio#2483.

In brief, say a PV has a flurry of activity, then goes to a value (say 0) and does not change for some time. With the Optimized PP, you would get the average during this flurry, say 50 and then the next bins will have no events, in which case the Optimized algorithm just returns the mean value of the last bin that had events, e.g. it would return 50. This means that data browser would show the value '50' for the entire duration that the PV does not change, even though the PV's last value was 0.

The optimized with last sample algorithm is based on the optimized one but if a bin does not have any events then it uses the last value in the previous bin that had events and hence in this example would return 0 as desired.

This new post processor was introduce to the CS-Studio data browser as a preference that could be switched on. In this PR I have added a similar preference to use the optimLastSample_ option.

At the moment I have left the default as 'false', i.e. it is not used but I wonder, given that this post processor gives a better representation of the data, whether it should be the default to use (i.e. make the variable true) - thoughts?

Checklist

  • Testing:

    • The feature has automated tests
    • Tests were run
    • If not, explain how you tested your changes
  • Documentation:

    • The feature is documented
    • The documentation is up to date
    • Release notes:
      • Added an entry if the change is breaking or significant
      • Added an entry when adding a new feature

@sonarqubecloud

Copy link
Copy Markdown

@shroffk
shroffk requested review from jacomago and shroffk August 24, 2026 17:30

@jacomago jacomago left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't it a bit limiting to make this a configuration parameter rather than replace the 'use_raw' on the trace to be an enum of 'optimized', 'optimizedWithLastSample', 'raw'. Do we really want for every PV optimizedWithLastSample over optimized? Or is it a case by case basis? I always think of a temperature sensor vs switch triggered, one is a continuous value that we sample in time whereas a switch triggered is an event that happens. Those two usually have a different way you want to think about 'last sample' included.

Also optimized is a bad name, we should try avoid keeping it. I think we should consider a better name that describes what it does, displayMinMaxMean or something. 'Optimized' doesn't say what it is optimized for.

protected void fetchDataInternal(String pvName) throws ArchiverApplianceException {
String optimized = new StringBuilder().append(ApplianceArchiveReaderConstants.OP_OPTIMIZED)
String optimizedOperator = ApplianceArchiveReaderConstants.OP_OPTIMIZED;
if (AppliancePreferences.ppOptimizedWithLastSample)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

switch maybe makes more sense

@@ -20,6 +20,7 @@ public class AppliancePreferences {
@Preference static boolean useStatisticsForOptimizedData;
@Preference static boolean useNewOptimizedOperator;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we instead be changing this to an enum?

@rjwills28

Copy link
Copy Markdown
Contributor Author

Thanks for taking the time to look at this and for the ideas. We could definitely expand the 'Request' option to be an enum and include the optimLastSample instead of just having essentially optimized = On/Off. A couple of points/questions though:

  • In response to whether we would always want optimLastSample given the two cases of a temperature sensor and a switch:

    • Wouldn't the switch be represented by an enum (e.g. On/Off) in which case the optimized/optimLastSample option doesn't get used anyway as it either falls back to raw or 'nth_' . So whether we're using optimized/optimLastSample wouldn't matter in this case.
    • And then the temperature sensor is a case where we might want to use optimLastSample to catch cases where this might not change for a while?

    It's not clear to me whether there is a case where you want 'Optimized' over 'OptimLastSample' but maybe I haven't missed something? The only difference in behvaiour between the two is when there are no samples in a bin.

  • Optimized name change: I agree it's not obvious what you are going to get when you select 'Optimized' but I thought the original use of the Optimized name was to signify that the data browser requests a reduced set of samples, i.e. it was more to do with limiting how much data gets sent back from the request in terms of memory usage? In terms of an alternative name, whether the min/max/stddev actually get displayed would depend on the 'Trace Type' so maybe that shouldn't be in the name of the 'Request'. Another option could be 'Binned'? If we did go down the route of making the 'Request' an enum then I'm not sure what you would call the 'optimLastSample' version - 'BinnedWithLastSample' isn't very clear.

@jacomago

Copy link
Copy Markdown
Contributor

Thanks for taking the time to look at this and for the ideas. We could definitely expand the 'Request' option to be an enum and include the optimLastSample instead of just having essentially optimized = On/Off. A couple of points/questions though:

You convinced me optimLastSample is just better 😀 Configuration parameter is then good. We can leave the renaming/documenting what 'optimized' means to another PR.

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.

2 participants