Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Float waveform PVs are displayed to the wrong precision #21

Closed
rjwills28 opened this issue Mar 12, 2024 · 5 comments
Closed

Float waveform PVs are displayed to the wrong precision #21

rjwills28 opened this issue Mar 12, 2024 · 5 comments

Comments

@rjwills28
Copy link
Contributor

We have been testing pvws/dbwr with an EPICS waveform PV of type FLOAT, e.g.

record(waveform, "TEST:ARR0")
{
    field(NELM,"10")
    field(FTVL, "FLOAT")
}

This PV is displayed as expected in Phoebus, i.e.
phoebus_float_waveform

However when the same BOB file is displayed in dbwr, the array values are given to a much higher precision which is misleading, i.e.
dbwr_float_waveform

I have checked the pvws response and confirmed that this misleading array is what is sent back to the client therefore I think the issue lies in pvws. Note that we do not see this issue when using a waveform PV of type DOUBLE.

pvws tries to handle a FloatArray in the same way as it does for a DoubleArray however this doesn't seem to work. I think the problem is coming from the inherent way that floats are stored meaning that the value can change from the original (i.e.. the 'float precision issue'). I have found a way around this which minimizes the impact on the rest of the code. This includes converting the float to a string and then parsing as a double.
I have forked and created a branch with this fix - please see: main...rjwills28:pvws:float_waveform_fix. I have created a new method to handle floats based on handleDoubles, which handles the float values differently (see line 264+265 in above branch).

@kasemir
Copy link
Collaborator

kasemir commented Mar 12, 2024

The example seems beyond the point. Your waveform record doesn't configure any display precision.
The widgets with default, pre -1 also don't configure anything.
So the widget may display the value any way it pleases.
Converting the float to a string and then parsing as a double seems like a hack to work around something that's undefined.

If you want to display floats with two digits, configure PREC=2 on the record. The PVWS then needs to send the number with display metadata that has precision=2, but the values are simply the "Float.toString" representation, since we do want to send the actual data, and the precision is a display hint.

Then make the widget use the precision to format the numbers.

@rjwills28
Copy link
Contributor Author

I have configured the float record to have PREC=3, e.g.

record(waveform, "TEST:ARR0")
{
    field(NELM,"10")
    field(PREC, 3)
    field(FTVL, "FLOAT")
}

The results are similar, even if I change the precision in the widget from -1 to 3 to dictate the precision. See example below:

caput
caput -a TEST:ARR0 10 2.346 3.45

Phoebus
phoebus_float_waveform_prec
DBWR
dbwr_float_waveform_prec

So is the issue actually with DBWR and that it is not yet using the precision specified in the widget (or the PREC field in the case precision is -1) to configure the display? I still think that the information sent from PVWS is misleading as it doesn't represent the actual data and if I were to set the precision of the widget to a higher number I would start to see those spurious numbers instead of 0s.

@kasemir
Copy link
Collaborator

kasemir commented Mar 13, 2024

There are two issues.

The main one is in the DBWR. The "precision" metadata isn't used when formatting waveform elements. Feel free to fix that.

The other one is in the PVWS. While we generally use JSON, arrays are sent as Base64 encoded binary, because that tends to reduce the packet size. We currently support "b64int" and "b64dbl". Char arrays are encoded as string (your other complaint #20), all integers are sent as b64int, and float as well as double are sent as b64dbl. The latter is why float values arrive with too many digits. Your suggested code change truncates the double values, but then still sends the float data as b64dbl, which is twice as large as a float array needs to be. The proper fix would be to add "b64flt" support, encode the float array as, well, duh, binary 32-bit IEEE float arrays. No float-to-string-back-to-double-then-b64dbl. Please add "b64flt" support to both Vtype2Json and the corresponding pwvs.js handleMessage. While at is, feel free to also add "b64srt" support for 16-bit shorts, since we're now sending all integers as 32 bit ints and are again losing any size advantage we'd get from 16 bit shorts.

rjwills28 added a commit to rjwills28/pvws that referenced this issue May 9, 2024
Addresses discussion in Issue ornl-epics#21 to support different array types.
@rjwills28
Copy link
Contributor Author

Thanks for the detailed feedback Kay. I have followed your suggestions and have just opened a set of PRs addressing these changes:

This looks cleaner now so I think these are worthwhile changes. However, this still does not fix the spurious precision for float arrays. Below I show the DBWR display after applying my above changes. I am comparing a waveform of type float vs a double.
dbwr_flt_vs_dbl
Both PVs have been set to [2.346, 3.45]. As you can see, with the precision set to '-1' (i.e. use the PREC field) the arrays are now displayed to the correct number of dps (3 for both). But if I set the precision preference in the BOB file to 8 then I get different results. The double array is simply filled with 0s, however spurious values are inserted for the float array. I actually see the same result in Phoebus.
I think this is a feature of floating point numbers that cannot be represented correctly. Perhaps we just say that it is up to the user to specify the correct precision? Perhaps people also tend to just use double type waveforms and not floats?

@kasemir
Copy link
Collaborator

kasemir commented May 10, 2024

Thanks for updating PVWS and DBWR to better handle the various array data types!

.. precision.. to 8 .. spurious values .. same result in Phoebus.
.. feature of floating point numbers that cannot be represented correctly.

Exactly. Check the basics of floating point numbers, like https://en.wikipedia.org/wiki/Single-precision_floating-point_format:
"All integers with 7 or fewer decimal digits.. can be converted exactly into an IEEE 754 single-precision floating-point value."

In other words, you can at most have 7 significant digits for a float.
See this Java example that shows how trying to add two numbers gives a slightly wrong result as we exceed 7 digits overall:

        // There is a "float" data type, because that's what was used to store
        // floating point numbers way back, but it's not very accurate.
        float f1 = 99999.8f;
        float f2 = 99999.65f;
        // This should compute to 199999.45, but will print .44
        System.out.println(f1 + f2);
        if (f1 + f2  ==  199999.45f)
            System.out.println("What are you talking about, looks good to me?!");
        else
            System.out.println("Houston, we really do have a problem...");

That's why almost all floating point math is now done in double, with maybe two exceptions:
Some embedded system that has no floating point unit, so float or even a fixed-point integer is much faster,
and array transfers. For the latter, you transfer float arrays when they're "good enough" and of course smaller = faster. When then using that data to compute stuff, you likely do that in double.

@kasemir kasemir closed this as completed May 10, 2024
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

No branches or pull requests

2 participants