Re: hardcoded median function in phosphor-virtual-sensor

[Matt Spinler <mspinler@linux.ibm.com>]

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@linux.ibm.com
>>> <mailto:mspinler@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.

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.

> 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.
>