Opened 5 years ago

Closed 4 years ago

#5618 closed defect (fixed)

Erroneous unit of sine phase angle

Reported by: ceraolo Owned by: adeas31
Priority: high Milestone: 1.16.0
Component: OMEdit Version: v1.14.0-dev-nightly
Keywords: Cc: Andrea.Bartolini

Description (last modified by ceraolo)

OMedit provides automatic conversion of sine wave phase angle from radians to degrees and vice versa.
this is nice, but in the current implementation has a tricky side-effect when symbolic parameters are used. Consider the following model:

model Test
import Modelica.Constants.pi;
  Modelica.Blocks.Sources.Sine sine1(phase = pi / 2)  annotation(
    Placement(visible = true, transformation(origin = {0, 0}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));
equation

annotation(
    Diagram(coordinateSystem(extent = {{-100, -80}, {100, 80}})),
    uses(Modelica(version = "3.2.3")));
end Test;

When we double-click on sine1 the phase is erroneously indicated as being degrees.

Attachments (1)

ExampleModifiers.mo (557 bytes) - added by casella 5 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by casella

As I understand it, if the modifier is a literal constant, then it should be converted and displayed from "unit" to "displayUnit". In case of expressions, we should display the content in "unit".

This case is borderline, in the sense that the expression is a constant, though not a literal constant. Maybe in this case we could choose to actually evaluate it and show it with "displayUnit".

@adeas31, @ceraolo, what do you think?

comment:2 Changed 5 years ago by ceraolo

I think that there are good reasons to leave symbolic constants symbolic.
I mean, anyone reading pi/2 understands exactly what it means as a sine argument, while 1,570796326794896 is cryptic and 1,57079 is inaccurate.
If, later I want to convert from pi/2 to pi/3, in case the constant is left symbolic is immediate, in the other it is not.

I understand that OMEdit when finds a literal constant in a sine argument, without unit or displayUnit, intends that that number is in radians, converts it into degrees, and allows the user to change it, defining the new value either in degrees or radians. It adds displayUnit accordingly. This is nice.

When OMEdit does not find a literal constant it obviously does not make any conversion.
Since it does not make any conversion, the unit stays unchanged (radians in this ticket's case).

So, I think that the solution of the ticket is straightforward: no conversion -> no addition of displayUnit -> use of default unit in the dialog box (i.e. rads, unless a different displayUnit was present)

Last edited 5 years ago by ceraolo (previous) (diff)

comment:3 Changed 5 years ago by casella

Sound good to me :)

comment:4 Changed 5 years ago by adeas31

  • Resolution set to fixed
  • Status changed from new to closed

comment:5 Changed 5 years ago by ceraolo

Thanks

comment:6 Changed 5 years ago by casella

Andrea.Bartolini reported me an issue which popped up after the commit in comment:4. Please load the attached package, open Test, switch to Diagram view, open the parameter window of m1, click ok, switch back to the text view. The parameter p1 now has a (displayUnit = "Pa") modifier.

I think the decision not to use the default display unit ("bar" in this case) in case the modifier is an expression should be taken on the fly by OMEdit, without changing the source code. We are trying as much as possible to avoid bogus source code changes for the benefit of revision control systems users, please stick to this goal.

Furthermore, with the current behaviour, I couldn't change the value of p1 from p1 to 3 bars (and found the behaviour of the GUI quite annoying).

comment:7 Changed 5 years ago by casella

  • Resolution fixed deleted
  • Status changed from closed to reopened

Changed 5 years ago by casella

comment:8 Changed 5 years ago by casella

  • Cc Andrea.Bartolini added

comment:9 Changed 5 years ago by adeas31

I tried to fix it 7bcccd3/OpenModelica. Can you try the nightly build tomorrow?

Last edited 5 years ago by adeas31 (previous) (diff)

comment:10 Changed 4 years ago by casella

  • Milestone changed from 1.14.0 to 1.16.0

Releasing 1.14.0 which is stable and has many improvements w.r.t. 1.13.2. This issue is rescheduled to 1.16.0

comment:11 Changed 4 years ago by ceraolo

  • Description modified (diff)

comment:12 Changed 4 years ago by ceraolo

  • Description modified (diff)

comment:13 Changed 4 years ago by ceraolo

It seems to me that this ticket can be reclosed.
@casella , since you reopened it, can you check?

comment:14 Changed 4 years ago by sjoelund.se

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.