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

Make constants available to type definitions #2746

Closed
wants to merge 4 commits into from

Conversation

henrikt-ma
Copy link
Collaborator

This PR breaks out the part of #2558 concerning the top level structure of a Flat Modelica description. The problem that needs to be solved is to make definitions of constants available to type definitions.

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Dec 3, 2020

In #2558 (comment), the following alternative solutions (here, cleaned up and renamed, but keeping original numbering) were proposed.

Design alternatives

Option 1: Global access to model constants

record 'R'
  Real 'x' = 'M'.'Modelica.Constants.pi';
end 'R';

model 'M'
  constant Real 'Modelica.Constants.pi' = 3.14; /* Will be available in the results. */
  Real 'x' = 2 * 'Modelica.Constants.pi';
end 'M';

Pros:

  • According to previous decision to not have any nested class definitions.

Cons:

  • Needs active hiding of constants not belonging to the original model's component hierarchy.
  • Change in the look-up rule.
  • Accessing constants outside the model looks ugly due to constants residing in the model 'M'.
  • Several top level definitions not allowed in full Modelica (but parser can be shared with full Modelica by use of semantic restrictions).

Option 2: All type definitions inside model

model 'M'
  record 'R'
    Real 'x' = 'Modelica.Constants.pi';
  end 'R';

  constant Real 'Modelica.Constants.pi' = 3.14; /* Will be available in the results. */

  Real 'x' = 2 * 'Modelica.Constants.pi';
end 'M';

Pros:

  • No change w.r.t. Full Modelica grammar (no need for semantic restrictions for full Modelica).

Cons:

  • Almost entire Flat Modelica description would be written with indentation by many users.
  • Poor separation of model's component hierarchy and equations from supporting class definitions.

Option 3: Top level constant component definitions

constant Real 'Modelica.Constants.pi' = 3.14; /* Not allowed in current Flat Modelica grammar. */

record 'R'
  Real 'x' = 'Modelica.Constants.pi';
end 'R';

model 'M'
  Real 'x' = 2 * 'Modelica.Constants.pi';
end 'M';

Pros:

  • Only small change in look-up.

Cons:

  • Several top level definitions not allowed in full Modelica (but parser can be shared with full Modelica by use of semantic restrictions).

Option 4: Global access to record constants

record 'Modelica.Constants'
  constant Real 'pi' = 3.14;
end 'Modelica.Constants';

record 'R'
  Real 'x' = 'Modelica.Constants'.'pi';
end 'R;'

model 'M'
  Real 'x' = 2 * 'Modelica.Constants'.'pi';
end 'M';

Pros:

  • Only small change in look-up.
  • Similar concept as package constants, easy to comprehend.
  • No need for hideResult annotation.

Cons:

  • References to constants derived from Full Modelica have to be properly handled.
  • Several top level definitions not allowed in full Modelica (but parser can be shared with full Modelica by use of semantic restrictions).

Option 5: Top level packages containing only constants

package 'Modelica.Constants'
  constant Real 'pi' = 3.14;
end 'Modelica.Constants';

record 'R'
  Real 'x' = 'Modelica.Constants'.pi;
end 'R';

model 'M'
  Real 'x' = 2 * 'Modelica.Constants'.pi;
end 'M';

Pros:

  • Only small change in look-up.
  • Known concept of package constants, easy to comprehend.
  • No need for hideResult annotation.
  • Would help to get rid of constants in records.

Cons:

  • Introduces package in Flat Modelica.
  • Separation of the constant name.
  • Several top level definitions not allowed in full Modelica (but parser can be shared with full Modelica by use of semantic restrictions).

Option 6: Overarching package

package _
  constant Real 'Modelica.Constants.pi' = 3.14;

  record 'R'
    Real 'x' = 'Modelica.Constants.pi';
  end 'R';

  model 'M'
    Real 'x' = 2 * 'Modelica.Constants.pi';
  end 'M';
end _

Pros:

  • Only small change in look-up.
  • Known concept of package constants, easy to comprehend.
  • No need for hideResult annotation.
  • Would help to get rid of constants in records.

Cons:

  • Introduces package in Flat Modelica.
  • Package's name is redundant given the model name (or vice versa).

Original poll

(Omitting some discussion from the original comment in #2558.)

Poll:
Option 1: Henrik (1)
Option 2: Gerd, Hans, Oliver, Martin, Adrian (5)
Option 3: Gerd, Hans, Oliver, Adrian, Henrik (5)
Option 4: Henrik (1)
Option 5: Martin, Adrian, Henrik (3)
Option 6: Gerd, Hans, Martin, Adrian, Oliver (5)

Comparison of top candidates

Option 3

  • Change in grammar, but could be handled by a semantic check later.
  • Avoids additional pointless package name.
    Option 6
  • Would work in existing tools.
  • Introduce something that makes the model look as if comes from a package.

At the end of #2558 (comment), there was also an inconsistent conclusion that now seems obsolete:
Either Option 3 "Allow constants in global scope" or Option 2 "Package constant and record being defined inside model".

Use case

Full Modelica model

model M
  constant Real n = 5;
  record R
    Real x(nominal=n);
  end R;
M end;

Flat Modelica model for option 3:

constant Real 'n_' = 5 "Global constant alias for 'M'.'n'";

record 'R'
  Real 'x'(nominal = 'n_'); /* Can't refer directly to 'M'.'n', so using global constant instead. */
end 'R';

model 'M'
  constant Real 'n' = 'n_'; /* Included for appearance in simulation result. */
end 'M';

@henrikt-ma
Copy link
Collaborator Author

Last comment on this matter in #2558 (comment) (omitting parts that are not relevant here):

Web Meeting:

Option 3 "Allow constants in global scope"

  • pro: Improves readability allowing to keep constants and types outside the model that do not originate from inside the model.
    • e.g. indentation
    • easier to find components declarations and equation
  • con: requires duplicates in the model to make them part of the results.
  • con: change in grammar in terms of each mo file is expected to contain only one definition
    • This

Option 2 "Package constant and record being defined inside model".

  • pro: no change to grammar
  • con: require "hide results" annotation
  • con: model becomes a mix of constants from different sources (package)

Mix of Option 2 and 3: "Allow constants in global scope and allow types in the model"

@henrikt-ma
Copy link
Collaborator Author

henrikt-ma commented Dec 3, 2020

Adding another design alternative according to previous comment:

Option 7: Allow type definitions inside model and top level constant component definitions

This design is a mix of option 2 and option 3, hence allowing for different uses.

Done like option 2:

model 'M'
  record 'R'
    Real 'x' = 'Modelica.Constants.pi';
  end 'R';

  constant Real 'Modelica.Constants.pi' = 3.14; /* Will be available in the results. */

  Real 'x' = 2 * 'Modelica.Constants.pi';
end 'M';

Done like option 3:

constant Real 'Modelica.Constants.pi' = 3.14; /* Not allowed in current Flat Modelica grammar. */

record 'R'
  Real 'x' = 'Modelica.Constants.pi';
end 'R';

model 'M'
  Real 'x' = 2 * 'Modelica.Constants.pi';
end 'M';

Pros:

  • Benefits of option 3 (Only small change on look-up).
  • Possible to avoid drawbacks of option 2 (Poor separation and indentation).
  • No helper variables needed to represent components from outside inside the model.
  • Flat Modelica proof of concept implementation doesn't need to support both variants.

Cons:

  • Drawbacks of option 3 (to be fully compliant; not needed for proof of concept implementation):
    • Parser still needs to be extended to be able to handle several top level definitions, but this does not necessarily require to different parsers.
  • More than one way of doing things is often considered poor design.

@henrikt-ma henrikt-ma added the MCP0031 Base Modelica and MLS modularization (MCP-0031) label Dec 3, 2020
@olivleno
Copy link
Collaborator

olivleno commented Dec 3, 2020

Web Meeting
Oliver: Being able to read Flat Modelica as is by any Modelica tool would be nice, but there might be so many changes along with introducing Flat Modelica that this is unrealistic anyways.

Hans: One could add: "package _dummy" if you don't want to change the parser.

Martin: Why invent something new if it is not necessary.

Kai: Why not make the parser broader and validate for the restricted case. This allows to generate better error messages. This is a good practice, not a bad design.

Martin: We could agree to always create a package with a name of the model and name the model "Model".

Henrik: It should be easy for users to agree on a common style of indentation.

Hans: Why so concerned about generated Flat Modelica.

Henrik: It's also about hand written code.

Poll:
Option 1: Henrik (1)
Option 2: Gerd, Hans, Oliver, Martin, Adrian (5)
Option 3: (Gerd), Hans, (Oliver), Adrian, Henrik (5)
Option 4: Henrik (1)
Option 5: Martin, Adrian, Henrik (3)
Option 6: (Gerd), (Hans), Martin, Adrian, Oliver, (Henrik) (6---)
Option 7: Gerd, Henrik

Conclusion:
Most votes for option 2.
Henrik: Still against 2. Would vote for 6 rather than 2.
Gerd: option 6 is like 7 plus package around it.
Hans: A bit weird but ok.
Henrik: Would prefer to have the name on the package.

Decision:
We continue with option 6.
We will need to define what names to use.

Henrik: Will be good to find the name in the top node.
Option 1: name used for package
package model_name
model _
Henrik

Option 2: identical
package model_name
model model_name
Gerd, Oliver, Hans

Option 3: empty
package model_name
model ''
none

Option 4:
package pkg_name
model model_name
Oliver

To Do:
Update the grammar [Henrik]

@henrikt-ma
Copy link
Collaborator Author

Let's also write down any drawbacks of this one (compared to option 2) next meeting:
Option 5:

within 'model_name';
model 'model_name'

@HansOlsson
Copy link
Collaborator

Let's also write down any drawbacks of this one (compared to option 2) next meeting:
Option 5:

within 'model_name';
model 'model_name'

I don't think we should more time on new proposals for that topic.
Note that within is only defined in file-systems mapping in Modelica - but not really part of the language; and some tools store Modelica classes in data-bases where within isn't useful.

@henrikt-ma
Copy link
Collaborator Author

Let's also write down any drawbacks of this one (compared to option 2) next meeting:
Option 5:

within 'model_name';
model 'model_name'

I don't think we should more time on new proposals for that topic.
Note that within is only defined in file-systems mapping in Modelica - but not really part of the language; and some tools store Modelica classes in data-bases where within isn't useful.

This looks like the same kind of within to me; tools not storing classes on file don't need this; they have their own ways of keeping things organized, and the name is present at the model anyway.

@olivleno
Copy link
Collaborator

Web Meeting:
PR approved.

fix line break and merge [Henrik]

@olivleno olivleno closed this Dec 10, 2020
@henrikt-ma
Copy link
Collaborator Author

I think this must have been closed by mistake; it doesn't make sense in view of decision above. Reopening.

@henrikt-ma
Copy link
Collaborator Author

I've corrected the mistakes on the branch now, but it seems like the Reopen and comment action with the comment above didn't work. Therefore, I'll just merge the branch and leave this PR in its strange closed state.

henrikt-ma added a commit that referenced this pull request Dec 10, 2020
Merging this branch according to decision in #2746 (comment).

The PR has been closed, by mistake, I assume.  Let's see what happens now to the state of the PR...
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

3 participants