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

displayUnit conversion results in many trailing zeros #8860

Closed
bilderbuchi opened this issue Apr 25, 2022 · 19 comments
Closed

displayUnit conversion results in many trailing zeros #8860

bilderbuchi opened this issue Apr 25, 2022 · 19 comments
Assignees
Labels
COMP/GUI/OMEdit Issue and pull request related to OMEdit
Milestone

Comments

@bilderbuchi
Copy link

Description

Using displayUnit on component parameters sometimes results in values with a large number of trailing zeros and a 1 in the last digit.
This reduces readability of the code, and maybe just stems from a string formatting problem

Steps to Reproduce

  • Open this model, where we added a display unit to rod.L:
model OM_8860
  Modelica.Mechanics.Translational.Components.Rod rod(L(displayUnit = "mm")) annotation(
    Placement(visible = true, transformation(origin = {-18, -30}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation

  annotation(
    uses(Modelica(version = "4.0.0")));
end OM_8860;
  • Switch to the diagram view
  • Open the properties dialog of rod
  • Enter a value of 30 (mm) for L (i.e. 1e-3 in base units)
  • Observe the message browser warning "Scripting Warning
    Could not preserve the formatting of the model instead internal pretty-printing algorithm is used." - I don't know if this is supposed to happen, may be related to Pretty-Printing algorithm being called after deleting components in OMEdit v1.18.1 #8635
  • The source code is now rod(L(displayUnit = "mm") = 0.03000000000000001)

Expected Behavior

After entering a length of 30 mm in the dialog, the entry in source should be 0.03, 30e-3, 3e-2 or similar.

AFAICT, the suggested additional precision was not there in the first place.
Note how Python behaves similarly when printing floats.

Version and OS

  • OpenModelica Version: v1.19.0-dev.beta1 (64-bit)
  • OS: e.g. Windows 10, 64 bit
@casella
Copy link
Contributor

casella commented Apr 25, 2022

Probably a duplicate of #7491.

The root issue was extensively discussed in #7465 in a slightly different context (flat Modelica output), though the discussion turned out a bit academic at a certain point. One solution is to use the routines from the Ryu library, which should address all such issues nicely. It seems that there was consensus on that, but then we didn't take action.

@sjoelund, @perost, @mahge, @adeas31 what would be the best way to incorporate this utility function in OMC, and then use it consistently for rendering of OMEdit unit-converted parameters and for (flat) Modelica output from the frontend?

@casella casella added this to the 1.20.0 milestone Apr 25, 2022
@bilderbuchi
Copy link
Author

Oops, yeah, this indeed looks like a dupe, sorry about that!

@casella
Copy link
Contributor

casella commented Apr 25, 2022

In fact, good that you raised this issue, that was left stranded as many others. I opened #8865 on this topic.

@paultjevdh
Copy link

Hi,

The reverse also seems to be true: the text model has a neat rounded value whereas the graphical view shows superfluous digits.
When I edit the value of a resistor which has displayUnit set to mOhm, the value is correctly stored in the text model, but the value in the graphical model and the parameter edit dialog has irrelevant digits.
For example setting a value of 1mOhm in the parameter edit dialog gives 0.9999999999999998mOhm when I close and re-open the parameter dialog. The value in the text model is 0.001.
What is particularly annoying in this case is that the zoom level of the graphical editor may cause truncation of the value string and it prefers to display all the irrelevant digits rather than the unit + scaling factor (milli, nano)
Quiz: what's the value of this resistor:
image
Apart from the discussion regarding whether it is sensible to make the user aware of the fact that rounding will occur, this should annoy mathematicians, physicists and engineers alike.
It's not trivial to do (I have done it), but I would like to have a routine that gives priority to scaling factors (milli, nano, ...) over digits.
If truncation occurs I suggest inserting the dots: R= 0.999...mOhm (if the value is something like 0.999234, or 1.000...mOhm when closer to 0.999723) That would acknowledge the occurrence of truncation/rounding and give the best possible representation of the number given the available space.

Best regards,
Paul
PS please tell me if this should better be posted to #8865 instead

@casella
Copy link
Contributor

casella commented Jun 2, 2022

please tell me if this should better be posted to #8865 instead

It's already mentioned there, thanks!

@casella casella modified the milestones: 1.20.0, 1.21.0 Dec 8, 2022
@casella casella modified the milestones: 1.21.0, 1.22.0 Apr 18, 2023
casella pushed a commit that referenced this issue Oct 3, 2023
…11168)

* use libryu.a library in MetaModelica to convert doubles to strings
- see tickets #8865, #8860, #7465

* fix the include path to ryu.h

* remove the bad *

* add using ryu in the cmake build

* use the functions from om_format.c moved inside libryu.a

* update 3rdParty to get the libryu.a changes

* fix the call to ryu OM routines
- use the new om_format.h header
- add the call to ryu to get the buffer
- declare the needed ryu functions as extern
- add the underscore back, it needs to be there
- update 3rdParty

* update and disable tests due to double printing format changes
- disabled tests (CHECK THESE PROPERLY!):
  testsuite/openmodelica/cppruntime/clockedEventRotationalTest.mos
  testsuite/simulation/libraries/msl32/Modelica.Electrical.Analog.Examples.AmplifierWithOpAmpDetailed.mos
  testsuite/simulation/modelica/others/EngineV6_output.mos
  testsuite/simulation/libraries/msl32/Modelica.Mechanics.MultiBody.Examples.Loops.EngineV6.mos
@casella casella modified the milestones: 1.22.0, 1.23.0 Nov 8, 2023
@casella
Copy link
Contributor

casella commented Jan 29, 2024

OK, current status with the original issue reported by @bilderbuchi and the current 1.23.0 nightly build:

  • The scripting warning is no longer issued
  • The value written in the Modelica code under Windows is still badly rounded, now I get: 0.030000000000000006, but it's ok in Linux.

The root cause is #11397. This should be fixed automatically once we switch to the new MSYS2 UCRT64 runtime, see #10939.

@casella casella added the COMP/GUI/OMEdit Issue and pull request related to OMEdit label Feb 16, 2024
@casella
Copy link
Contributor

casella commented Feb 16, 2024

@bilderbuchi this should be now fixed with the 1.23.0-dev nightly build. Please check.

@adeas31
Copy link
Member

adeas31 commented Feb 16, 2024

Yes, in most of the cases it is working. However, I found a corner case.

package Test
  model M
    parameter Modelica.Units.SI.Density d[1];
    annotation(
      Icon(graphics = {Rectangle(origin = {-1, 0}, extent = {{-99, 100}, {101, -100}}), Text(origin = {4, 11}, extent = {{-66, 53}, {66, -53}}, textString = "M")}));
  end M;

  model S
  M m(d (each displayUnit = "kg/m3")= {950})  annotation(
      Placement(visible = true, transformation(origin = {0, -2}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
  end S;
  annotation(uses(Modelica(version="4.0.0")));
end Test;

OMEdit shows 949.9999999999999 in this case instead of 950. Ryu gives 949.9999999999999 for 949.99999999999989.

@casella
Copy link
Contributor

casella commented Feb 16, 2024

This is weird. There is actually no conversion involved in this case: 950 has an exact representation in IEEE 754 double precision, so multiplying or dividing it by 1.0, which also has an exact representation, should still return an exact integer value. Why should it be rendered differently?

@adeas31, @perost could you please track all the steps from the 950 in the source code to the displayed 949.9999999999999, so we understand exactly where it goes wrong?

@perost
Copy link
Member

perost commented Feb 16, 2024

This is weird. There is actually no conversion involved in this case: 950 has an exact representation in IEEE 754 double precision, so multiplying or dividing it by 1.0, which also has an exact representation, should still return an exact integer value. Why should it be rendered differently?

@adeas31, @perost could you please track all the steps from the 950 in the source code to the displayed 949.9999999999999, so we understand exactly where it goes wrong?

From the OMEdit log I'm getting:

getModelInstance(Test.S,"",false)
> JSON with the binding as [950]
convertUnits("kg/m3","g/cm3")
> (true,1000,0)
convertUnits("g/cm3","kg/m3")
> (true,0.001,0)

I'm not sure about the purpose of those convertUnit calls, is OMEdit converting the value from kg/m³ to g/cm³ and then back again? That could maybe be the reason for the issue, specifically multiplying by 0.001 which doesn't have an exact representation.

@adeas31
Copy link
Member

adeas31 commented Feb 16, 2024

The actual displayUnit of Modelica.Units.SI.Density is g/cm3 so 950 is first converted to it i.e., 0.95 and then since the component has its own displayUnit specified as modifier i.e., kg/m3 so we convert it back to 950 (this time the rounding fails).

But in general this shouldn't be a problem. Multiple conversions should work fine. The same behavior can be achieved by just changing displayUnits multiple times from the dropdown.

@casella
Copy link
Contributor

casella commented Feb 16, 2024

I agree that we should have a system in place that can handle the rounding of multiple conversions nicely, though I find it sub-optimal that in case there is a local displayUnit we don't go for that directly. But that's another story.

So, apparently this pseudo code prints 949.9999999999999?

d = 950;
d = d/1000;
d = d/0.001;
print_ryu(d);

Could you please check that?

@casella
Copy link
Contributor

casella commented Feb 16, 2024

@adeas31 I think I know what is going on here. The problem is that you are carrying out two conversions in a row, which both introduce rounding errors. The first time you are computing d = 950.0/1000.0: the two operand are represented exactly in IEEE 754, but the result isn't, so that introduces some rounding error. Then you compute d = d / 0.001, whereby d at the RHS is approximated, and 0.001 also is. In theory, the result should be representable as an exact number (950), but in practice thanks to Murphy's law the two approximations work in the same direction, so you get 950 - eps instead.

Bottom line: please avoid the double conversion and do the conversion from unit to displayUnit only once, so we have a chance that Ryu can recover the round figure. In this specific case, there won't be any conversion, so it will work for sure.

The same behavior can be achieved by just changing displayUnits multiple times from the dropdown.

AFAIU this is not exactly the case. When you do this, each time the result of the operation is passed through Ryu, which tries to approximate it "on the right side", i.e. the one with less digits. This doesn't happen in the case at hand, where two conversions are applied in a row without any Ryu re-adjustment in-between.

@bilderbuchi
Copy link
Author

bilderbuchi commented Feb 19, 2024

@bilderbuchi this should be now fixed with the 1.23.0-dev nightly build. Please check.

I can confirm that the originally reported model shows fixed behaviour now.
(This does not affect the corner case you subsequently discovered, of course, so it's too early to close I think)

@adeas31
Copy link
Member

adeas31 commented Feb 19, 2024

I agree that we should have a system in place that can handle the rounding of multiple conversions nicely, though I find it sub-optimal that in case there is a local displayUnit we don't go for that directly. But that's another story.

So, apparently this pseudo code prints 949.9999999999999?

d = 950;
d = d/1000;
d = d/0.001;
print_ryu(d);

Could you please check that?

This produces 949.9999999999999 but also the following does the same,

d = 0.95;
d = d/0.001;
print_ryu(d);

So i don't think the double conversion is an issue here.

@casella
Copy link
Contributor

casella commented Feb 20, 2024

but also the following does the same,

d = 0.95;
d = d/0.001;
print_ryu(d);

So i don't think the double conversion is an issue here.

d = 0.95 produces an approximated Real representation. 0.001 also has an approximate representation. So, when you take the ratio d/0.001 there are two approximations there. If you are lucky, you get a representation which is compatible with 950, if you aren't, you get 950*(1+/-eps).

I guess the problem is that OMEdit does the unit conversions with factors that don't have an exact representation as IEEE 754 double precision numbers, such as 0.001 or 1e-6 etc. As in the above example, this introduces two approximations: one when representing the user input, e.g. 0.95, and one representing the conversion factor, e.g. 0.001.

I think we should make the unit conversion smarter, by trying to always have a conversion factor that has an exact representation. For example, if you now want to convert g/cm3 to kg/m3, you divide by 0.001, but you could actually multiply by 1000 instead. I think that

d = 0.95;
d = d*1000;
print_ryu(d);

would return 950. We could modify the convertUnit API so that it return a factor which is always >=1 and an additional boolean flag that indicates wheter you must multiply or divide by that factor to convert the unit.

I think this should fix it.

In any case I would try to avoid the double conversion, it is just an unnecessary numerical hazard.

@casella
Copy link
Contributor

casella commented Feb 20, 2024

We could also improve print_ryu(d) as follows

  • convert d to x.xxxeyy using ryu, as currently done
  • let rd = round(x.xxxxx*1e14)/1e14
  • convert rd to z.zzzzeww using ryu
  • if the number of digits of z.zzzzz is at least, say, six digits less than x.xxxx, then continue with rd, otherwise continue with d

However, this is somehow based on heuristics. I believe the right approach is to do the conversions by multiplying and dividing by the same factor, preferably (when possible) with an exact representation (e.g. an integer value), rather than dividing by a factor and then dividing by it inverse.

@adeas31
Copy link
Member

adeas31 commented Mar 4, 2024

Works as expected after #12052

@casella
Copy link
Contributor

casella commented Mar 4, 2024

Confirmed

@casella casella closed this as completed Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/GUI/OMEdit Issue and pull request related to OMEdit
Projects
None yet
Development

No branches or pull requests

5 participants