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

Removed min=0 attribute from SIunits.Height #2639

Closed
wants to merge 1 commit into from

Conversation

casella
Copy link
Contributor

@casella casella commented Jun 15, 2018

Modelica SIunits currently defines Height as

type Height = Length(min=0);

IMO negative height can make sense in a number of cases, e.g.

  • height above mean sea level (can be negative in the Netherlands or on the Dead Sea shore)
  • hydraulic head of a pump (can be negative if the inlet pressure is greater than the outlet pressure)

In fact, the latter example actually influences the pump models in both Modelica.Fluid and ThermoPower, and unnecessarily introduces the constraint that the head should be positive, which is in general unnecessarily restrictive.

I would then propose to simply remove the min attribute (which I consider as a bugfix). As we are not introducing additional constraints, this change should be 100% backwards compatible.

@beutlich beutlich added L: SIunits Issue addresses Modelica.SIunits discussion Discussion issue that it not necessarily related to a concrete bug or feature labels Jun 15, 2018
@beutlich
Copy link
Member

This reminds me of #446 on SIunits.Distance.

Not sure if we should touch this now. For me, it seems odd to change it in Height, but not in Breadth.

The library officers should really review this.

@HansOlsson
Copy link
Contributor

To me this seems like a larger change, and not something we just add during beta-testing.

There are also several min-values in Modelica.SIUnits that could possibly be changed in the same way (negative Resistance is used in some cases - similarly negative Density when using a density comparison) ; the simple solution would be to use "Position" instead of Height for the ones above - or use a modifier to override min=0.

@HansOlsson
Copy link
Contributor

HansOlsson commented Jun 15, 2018

An additional comment is that "height above sealevel" can be called "altitude" or "elevation" (slightly different meaning - and depending on context as well).

For the Pipes in MSL I am not sure about the design we have with respect to this:

  parameter SI.Length length "Length";
  parameter SI.Diameter diameter "Diameter of circular pipe";
  parameter SI.Area crossArea=Modelica.Constants.pi*diameter*diameter/4;
  parameter SI.Length perimeter=Modelica.Constants.pi*diameter;
  parameter SI.Height roughness=2.5e-5;
  parameter SI.Length height_ab=0 "Height(port_b) - Height(port_a)";

I am not sure about the design - the height difference can be negative, but so can the length and perimeter but not diameter and roughness.

For pumps it is not only head and head_init_single that should be changed from Height to Length; but also dp_pump is described as "pressure increase"; whereas to allow negative head we should say it is "pressure change".

@MartinOtter
Copy link
Member

Basically, Hans is right (but pointing to the wrong type): If min=0 is not meaningful, use Length instead. Distance, Breadth, Height, Thickness, Radius, Diameter are all Length types with min=0, and it seems not justified to quite arbitrarily pick one of them (Height) and remove min, if the Length type is avaiable

@beutlich
Copy link
Member

It is

type Position = Length;

thus, both types will work out.

@MartinOtter
Copy link
Member

Yes, both can be used. But in the same way as "Diameter" and "Height" means something slightly different, "Position" usally characterizes a coordinate in a coordinate system, whereas "Length" characterizes the dimension of a part.

@casella
Copy link
Contributor Author

casella commented Jun 15, 2018

In fact, if Lenght characterizes the dimension of a part, as Martin rightfully remarks, it should also have min = 0. On the other hand, "negative focal lenght" is a perfectly sound technical concept.

What I mean is that while Radius, Breadth, Width, Thickness and Distance are unequivocally non-negative concepts (as far as my non-native knowledge of English goes), Length and Height could also be negative (e.g. negative focal length, negative height over sea level, or negative pump head), depending on the context. In dubio pro reo, so I would prefer a more lax approach, just in case.

Regarding Height_ab in pipes, Height here refers to a vertical dimention, I really find it odd that I need to change it to Length to make it to work properly.

Maybe a solution would be to add RelativeHeight without min = 0 in MSL 3.2.3 and use it in pumps and pipes. This is guaranteed to have zero side effects. Would that be ok?

In any case, the current situation si unacceptable since currently we can't have a legal pipe model running downwards, which is obviously a bug that should be removed from MSL 3.2.3, so I would ask @MartinOtter to come up with a decision regarding this before the release. I can take care of the implementation.

IMO the persistence of these bugs has pushed the tool vendors to basically ignore min and max attributes in order not to bother end-users, possibly generating warnings when they are violated, while the specification clearly implies that an errorLevel assertion should be triggered if they are not satisified. This is not good, as these attributes can help a lot avoiding numerical issues or identifying them, so we should strive to avoid having situations in the MSL where a strict (i.e. correct) implementation of min/max would make models violating them invalid.

So, I'm fine with whatever solution we adopt (though I prefer using Height or RelativeHeight over Length as a type for vertical quantities), but we definitely need to do something about this for MSL 3.2.3. Last chance 😃

@HansOlsson
Copy link
Contributor

RelativeHeight would not be ok, since many other Relative types in Modelica.SIUnits are fraction (like RelativeDensity, RelativePermittivity). We normally use Difference for differences.

Using Length seems simpler (which is already used in Modelica.Fluid for similar cases as I noted above - however inconsistently); I don't agree that Position would have been wrong - just view it as a coordinate system where the "head" goes in the y-direction; but I don't see a reason to introduce a new type.

@beutlich
Copy link
Member

beutlich commented Jun 15, 2018

I also propose to use Position, same way as #446 was fixed.

@beutlich beutlich added this to the MSL_next-MAJOR-version milestone Jun 24, 2018
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Feb 18, 2019
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Mar 2, 2019
Forgot to commit previously.
@beutlich
Copy link
Member

Superseded by #2834.

@beutlich beutlich closed this Mar 22, 2019
@beutlich beutlich modified the milestones: MSL4.0.0, never Mar 22, 2019
@beutlich beutlich added invalid Invalid issue and removed discussion Discussion issue that it not necessarily related to a concrete bug or feature labels Mar 22, 2019
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Apr 4, 2019
@casella casella deleted the FixHeightType branch May 10, 2019 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Invalid issue L: SIunits Issue addresses Modelica.SIunits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants