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

Flat modelica type aliases #2555

Merged
merged 8 commits into from
May 3, 2022
Merged

Flat modelica type aliases #2555

merged 8 commits into from
May 3, 2022

Conversation

henrikt-ma
Copy link
Collaborator

Initiating draft PR for design work on type aliases.

@olivleno
Copy link
Collaborator

olivleno commented Apr 28, 2020

Wouldn't it be useful to have a Full Modelica code for all the examples so that one can see how this is transformed?

Referring to:

No type aliases for records

record PositiveInterval
  Interval member(low(min = 0.0), high(min = 0.0));
end PositiveInterval;

I don't see the benefit of one type being defined by another type.
One could easily unroll the type declaration with repeated code but enhanced understandability.

record PositiveInterval
  Real low(min=0.0);
  Real high(min=0.0);
end PositiveInterval;

Flat Modelica is not for building libraries! It's for stand-alone ("save all" kind of) models.
Getting rid off hierarchical dependencies is one of the main benefits of Flat Modelica, isn't it?

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Apr 29, 2020

Web meeting poll: How to proceed?

  • Make detailed design now: OL
  • Decide to allow full power of modifications: HT, GK
  • Postpone until simplification of modifications is settled: MS, HO
  • Abstain: KW

Conclusion: Postpone.

@HansOlsson HansOlsson added the MCP0031 Base Modelica and MLS modularization (MCP-0031) label May 8, 2020
@henrikt-ma
Copy link
Collaborator Author

This PR seems to contain way to many changes that don't focus on type aliases. We should have a look at the suggested changes to see if there is anything worth preserving.

Regarding where we stand on this issue, I think that the type alias topic itself is greatly simplified by the current top level structure, which only gives access to top level constants for expressing modifications in the type aliases, and we've anyway gotten rid of pretty much all non-constant built-in type attributes. That is, type aliases can only created easily handled variants of existing types, plus making arrays of existing types.

I also get the impression that if we want to focus on type aliases we need to do so in a fresh PR; there's way too much unrelated discussion above. Maybe we could just make a new decision between these two options?

  • Consider type aliases simple enough for now.
  • Open new PR with strict focus on type aliases.

@henrikt-ma
Copy link
Collaborator Author

When looking at #2468 I realize that the big question where we seem to disagree regarding type aliases is whether to allow type aliases for arrays (this would be the only way to declare an array type).

updated the example code in scalars section
@olivleno
Copy link
Collaborator

Web Meeting: Martin, Hans, Henrik, Gerd, Oliver
Continue this PR with a strong focus on allowed type aliases, especially without diving into "one way of array declarations" ( #2468).

Goal:
Avoid time varying types.

Conclusion for scalars:
No alternatives discussed, only one design as documented in
RationaleMCP/0031/type-aliases.md

updated the example code

Since `start` has been removed as an attribute in Flat Modelica except as syntactic sugar, it is better to use use one of the actual attributes for this discussion.  Avoiding `nominal` since it has some similarity with `start` and would need to be removed from the set of built-in attributes if we wanted to support the non-constant nominal attribute from FMI.
@olivleno
Copy link
Collaborator

olivleno commented Nov 2, 2021

Web-Meeting: Martin, Hans, Henrik, Gerd, Oliver

Rewrite the "No type aliases for records" example to illustrate workarounds to avoid dummy member.

Workaround 1:

record PositiveInterval
  Real low(min = 0.0);
  Real high(min = 0.0);
end PositiveInterval;

Interval interval1;
PositiveInterval posinterval1;
PositiveInterval posinterval2;

Workaround 2:

Interval interval1;
Interval posinterval1(low(min = 0.0), high(min = 0.0));
Interval posinterval2(low(min = 0.0), high(min = 0.0));

Shall replace the current example in type_aliases.md [Henrik]

Discussion:

No type aliases for records

Henrik: Scales extremely bad.
Hans: It depends how much commonalities are present in the used types.
Henrik: An alias is very convenient and a comparatively simple extension.

@henrikt-ma
Copy link
Collaborator Author

Examples have been changed as decided above: #2555 (comment)

@olivleno
Copy link
Collaborator

olivleno commented Nov 9, 2021

Web Meeting: Hans, Martin, Gerd, Henrik, Oliver

Decision on type aliases requires a decision on what types shall be abel to represent.

What types shall be allowed in Flat Modelica?

  1. Remove value modifications in types:
record R
  Real x=1;
  Real y;
end R;

record R2
  R r(y=2);  //forbidden
end R2;

To be continued.

@olivleno
Copy link
Collaborator

Web Meeting: Hans, Martin, Gerd, Henrik, Oliver

What types shall be allowed in Flat Modelica? (continued from above)

  1. Forbid types with value modifications:
record R
  Real x=1;  //forbidden
  Real y;
end R;

record R2
  R r(y=2);  //forbidden
end R2;

All participants of the meeting agree that there is no need to support value modifications in Flat Modelica types.
This will simplify semantics of record construction and we could similarly simplify functions by disallowing default values.
@perost : What do you think about this?

Next meeting:
Solve the prerequisite question about record constructors and function default arguments first.
Open a new PR for the new related roadmap line item. [Henrik]

Thursday Jan. 13, 10:30 - 12:00

@perost
Copy link
Collaborator

perost commented Nov 18, 2021

All participants of the meeting agree that there is no need to support value modifications in Flat Modelica types. This will simplify semantics of record construction and we could similarly simplify functions by disallowing default values. @perost : What do you think about this?

That's fine for OpenModelica, type modifiers and default arguments are applied in the flat model and serve no real purpose. We currently do allow them since they're part of regular Modelica, but it doesn't really matter much to us if we allow or forbid them.

@olivleno
Copy link
Collaborator

Web Meeting: Martin, Hans, Henrik, Gerd, Oliver
(after simplified value modifications decision)

Solve the prerequisite question about record constructors and function default arguments first.
Open a new PR for the new related roadmap line item. [Henrik]

This has been solved during the discussion of value modifications by having removed record constructors and function default arguments entirely.

Web Meeting: Martin, Hans, Henrik, Gerd, Oliver Continue this PR with a strong focus on allowed type aliases, especially without diving into "one way of array declarations" ( #2468).

Goal: Avoid time varying types.

Conclusion for scalars: No alternatives discussed, only one design as documented in RationaleMCP/0031/type-aliases.md

updated the example code

This goal is achieved by restrictions applied to modifications.

The above question about which type aliases to allow was strongly dependent on which modifications may occur. This should be easy to answer now.

Conclusion from having simplified modifications:

  • Very complicated types using records were possible. This is much restricted for long record class definitions and even simpler for record type aliases, as only package constants are in scope.
  • record type aliases should be allowed as their modifications are so restricted now. Hence, they are easy enough to handle.
  • Merging is simplified as there is no-longer a need to trace back where modifications came from with having the global scope only.

Proposal:
record type aliases should be allowed in flat Modelica.

Poll:
in favor: Henrik, Gerd, Hans, Martin, Oliver
against: -
abstain: -

Decision:
record type aliases are allowed in flat Modelica.

@olivleno
Copy link
Collaborator

Web Meeting: Hans, Henrik, Gerd, Martin, Oliver

See decision to not allow array type aliases at #2468.

@henrikt-ma
Copy link
Collaborator Author

This PR is currently a mess with 12 changed files. I'll have to try to restart it so that we can see clearly the changes that relate to type aliases.

@henrikt-ma henrikt-ma marked this pull request as ready for review May 2, 2022 20:55
@henrikt-ma
Copy link
Collaborator Author

It turned out that all there is on this PR branch is the new supplemental design document type-aliases.md. The actual restriction to not allow array type aliases is taken care of in #2468.

@olivleno olivleno merged commit ecdc3df into MCP/0031 May 3, 2022
@olivleno olivleno deleted the MCP/0031+type-aliases branch May 3, 2022 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MCP0031 Base Modelica and MLS modularization (MCP-0031)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants