hardcoded median function in phosphor-virtual-sensor

[Ed Tanous]

On Tue, Jan 5, 2021 at 7:59 AM Matt Spinler <mspinler at linux.ibm.com> wrote:
>
>
>
> On 1/5/2021 8:27 AM, Patrick Williams wrote:
> > On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
> >> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
> >>> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler at linux.ibm.com
> >>> <mailto:mspinler at linux.ibm.com>> wrote:
> >>>
> >>>      I need a median of some sensor values, where this median sensor has
> >>>      threshold interfaces
> >>>      whose values must be defined in entity-manager.  Since exprtk
> >>>      expressions are not allowed in
> >>>      entity-manager, I cannot just port the PVS's JSON config into an
> >>>      entity-manager config.
> >>>
> >>> I missed this discussion but why can't we simply use virtual sensor as
> >>> expertk provides median function and we have threshold support for
> >>> each virtual sensor. Please help, if I am missing anything.
> >> If you're asking why can't we just have PVS get its exprtk expression to
> >> use from entity-manager, and encode the median there, it's because Ed
> >> doesn't want exprtk in entity-manager config files (I'll defer to him on
> >> the reasons).
> > As part of offline discussions on this I said that having a one-off EM
> > config for median to allow you to make progress is reasonable, but I
> > don't think having a bunch of one-offs is a viable long-term solution
> > for phosphor-virtual-sensors.  Basically we're kicking the can down the
> > road until we have a better understanding of what kinds of EM/PVS
> > configs we're going to need.
> >
> >> If you're asking now that the median function is hardcoded, why write it in
> >> C++ instead of exprtk, it's because the exprtk code would be so similar to
> >> C++ code (skip the bad values, put the sensors in a vector,  call
> >> nth_element)
> >> that writing it in exprtk doesn't really buy us anything, and using C++ lets
> >> me skip making the symbol table.
> > I would certainly prefer we use the exprtk code here.  You should be
> > able to generate (at runtime) a exprtk expression from the EM config you
> > specified below.
> >
> > My rationale is:
> >      * Ed suggested that a long-term answer might be a few lookup tables
> >        / transformations from the EM configs to the PVS expressions.  I'm
> >        not fully bought into this yet but it seems like a reasonable
> >        direction to explore.
> >
> >      * You wrote "because the exprtk code would be so similar to the C++
> >        code", which is why you *should* do it in exprtk.  The rest of the
> >        PVS code is already this way so why something different and
> >        require dual maintanence?  If the exprtk-based support code is
> >        missing some of these features, lets add them because others will
> >        likely need them as well.
>
> See below.
>
> >>>      Instead, I will make a new entity-manager config that will have the
> >>>      component sensors
> >>>      along  with the thresholds to use, with a subtype of median, vaguely
> >>>      something like:
> >>>
> >>>      {
> >>>      Type: "VirtualSensor"
> >>>      Name: "MySensorName"
> >>>      Subtype: "Median"
> >>>      Sensors: [ "Sensor1", "Sensor2", .... ]
> >>>      ThresholdsWithHysteresis [ ]
> >>>      minInput: 0
> >>>      maxInput: 100
> >>>      }
> >>>
> > I would expect the 'exprtk' expression from your EM config to be
> > something like "median(Sensor1, Sensor2...)".  You should be able to
> > feed this into the existing virtual-sensor constructors and not have to
> > make the symbol table yourself.
>
> Every variable used by exprtk needs to be added to the symbol table
> first by the C++.
>
> Also, we need a slightly tweaked median of our 3 ambient temp sensors:
> 1) throw out values outside of minInput/maxInput
> 2) if there is an even number, because we threw out one, choose the
> higher value, and
>      don't do the average of the 2  that I believe an actual median
> would use.
> 3) if we threw out all 3 (very unlikely), use NaN as the sensor value.
>

These kinds of corner cases are exactly why IMO C++ is easier in the
long run.  Those 3 conditions are trivial to add to a C++ based
daemon, but would require a lot of complex expertk code to define if
the corner cases were found later.  In C++ they just end up as an
extra branch.

> This is easy to do in C++ using std::nth_element, and basically looks
> the same in
> exprtk which is why I suggested just using C++ though I don't really
> care that much either way,
> but I don't see how we could upstream this as a true median().  In fact,
> since
> the underlying code would just use  nth_element anyway, I'm not even
> sure it would
> be accepted and is probably why there isn't already a median().
>
> Since I guess it could be argued this isn't a true median, maybe we
> shouldn't call it
> a median, which is fine, but we still need it.

Maybe we could call it "AirIntakeTempAggregator" or something of that sort?

>
> > Exprtk doesn't currently support a 'median' operator but it does support
> > 'avg'.  We should contribute this upstream and add as a patch in the
> > meantime.
> >
>