search

modelica / ModelicaSpecification

Specification of the Modelica Language
https://specification.modelica.org
Creative Commons Attribution Share Alike 4.0 International
104 stars 40 forks source link

MCP-0019: Flattening #1829

Closed modelica-trac-importer closed 6 years ago

modelica-trac-importer commented 6 years ago

Reported by kurzbach on 10 Nov 2015 08:34 UTC This MCP is about the improvement of chapter 5.6 "Flattening Process" of the specification.

In the current Modelica language specification (MLS 3.3 revision 1) the procedure how to get the right DAE system of equations from a given model is not well defined. There are several ambiguities and understanding problems, resulting from the very short and abstract description.

The problems and missunderstandings are documented in several bug tracker tickets. Here's a list of tickets that can be summarized as "instantiation is not sufficiently defined":

638, #641, #661, #709, #789, #791, #841, #862, #863, #1033, #1043, #1081, #1219, #1314, #1397, #1458, #1574, #1645, #1649, #1654, #1680, #1685, #1687, #1696, #1716, #1734, #1735, #1745, #1772,

Out of these most of the discussion is on #1397 and #1458, but all of these tickets (and possibly more) contain important questions that needs to be answered when specifying the instantiation.

Because flattening is the central procedure when translating Modelica code into equations and finally to a simulatable model, the drawbacks have the consequence that some models are not really portable, and the Modelica language cannot be said to be well standardized.

Therefore this MCP tries to specifiy the flattening more clearly in an algorithmic, stepwise approach, taking also the generation of equations into account.

Location of the documents: https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening

The current status is that the chapter 5.6 should be complete (except of inclusion of your comments), but there are a number of chapters still need changes to be consistent with this new description, especially parts of chapter 7 should probably merged into chapter 5.

Please use this ticket for discussion and comments. This topic is important and urgent, so it should be discussed also on the next design meeting in Munich.

The current members of the working group are:

Thanks to Peter Fritzson who gave the initial push to make it.

Migrated-From: https://trac.modelica.org/Modelica/ticket/1829

modelica-trac-importer commented 6 years ago

Comment by kurzbach on 15 Nov 2016 10:34 UTC In preparation of the web meeting today and as base for discussion I added a SpecChanges file with some comments and questions to the svn (https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges_GK.doc) (the version with "partially").

I tried to update the specification proposal on the svn-server with the ideas above, and also added a variant where I replaced "partially instantiated" with "instantiated" as discussed above in r9516.

In principle the omission of "partially" seems to work.

modelica-trac-importer commented 6 years ago

Comment by perost on 17 Nov 2016 14:42 UTC One thing I think needs some more consideration is where to check for duplicate elements, because it can have big implications. In the current proposal this is done at the end of the instantiation, while in comment:43 by Hans it's implied that it should be done earlier.

I guess it comes down to how strict we want the definition of "identical" to be. If identical means syntactically identical (within reason) it would be possible to check for duplicate elements while merging inherited elements into the current instance, which would simplify things since an instance would never have elements with the same name.

If we use a looser definition of identical it might not be possible to check for duplicate elements until after a class has been fully instantiated. This leads to some issues like the possibility of finding multiple elements with the same name during lookup. And just using the first element we find might not be enough, since classes which are only used for lookup might not need to be fully instantiated.

Consider this example:

model A
  model X end X;
end A;

model B
  extends A;
  model X end X;
end B;

model C
  B.X x;
end C;

B only needs to be instantiated enough that lookup can be done. If the check for duplicate elements is done early we could just check that the two X classes are syntactically identical and only keep the first one. Otherwise the check might never be done, since B might never be fully instantiated.

There's also this to consider:

model A
  Real x = 1.0;
end A;

model B
  extends A;
  Real x = 2.0;
end B;

model C
  extends B(x = 3.0);
end C;

B by itself is not legal to instantiate, but one could argue that the modifier in C overrides the modifiers on both of the x's in B so that they become identical. The current proposal says that extends clauses are instantiated and then their elements are inserted into the current instance. I'm not sure if this is a mistake or not, but I don't think extends clauses needs to be fully instantiated in this step. It would then be possible to handle extends first, and then merge modifiers to both local and inherited elements at the same time.

Doing so would make it possible to take each modifier, find the element it belong to and then merge it. This means that finding out if a modifier refers to a nonexisting element comes for free. Doing it the other way around, i.e. going through each elements and checking if there exists a modifier for it, means that you have to keep track of if there are unused modifiers. It should also be more efficient to go through the modifiers and finding out which elements they belong to than the opposite, since there's generally more elements than modifiers.

For what it's worth, OpenModelica does not currently allow this model.

And finally, should this be allowed?

model C
  Real x = 1.0;
  Real x = 1.0;
end C;

I can't think of any practical reason for why this should be allowed, but the way that the spec is currently written (i.e. section 4.2) is unclear regarding this.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 5 Dec 2016 16:49 UTC Replying to [comment:51 kurzbach]:

In preparation of the web meeting today and as base for discussion I added a SpecChanges file with some comments and questions to the svn (https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges_GK.doc) (the version with "partially").

I have tried to answer these (in some cases skipping simple "Agreed"; I have not deliberately missed any comments that I disagree with) in r9547. Assuming that is clearer we could update the document.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 9 Dec 2016 14:14 UTC Replying to [comment:53 hansolsson]:

Replying to [comment:51 kurzbach]:

In preparation of the web meeting today and as base for discussion I added a SpecChanges file with some comments and questions to the svn (https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges_GK.doc) (the version with "partially").

I have tried to answer these (in some cases skipping simple "Agreed"; I have not deliberately missed any comments that I disagree with) in r9547. Assuming that is clearer we could update the document.

The document is now updated in r9557

https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges_HOUpdatedGK.doc

modelica-trac-importer commented 6 years ago

Comment by perost on 12 Dec 2016 12:31 UTC Replying to [comment:54 hansolsson]:

The document is now updated in r9557

https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges_HOUpdatedGK.doc

I reviewed this and added some comments/questions in MCP_0019_Flattening_SpecChanges_POe.doc. The document was starting to get a bit cluttered though, so I only kept my changes and comments.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 12 Dec 2016 16:20 UTC Replying to [comment:55 perost]:

I reviewed this and added some comments/questions in MCP_0019_Flattening_SpecChanges_POe.doc. The document was starting to get a bit cluttered though, so I only kept my changes and comments.

Reviewed changes. Documents is updated, but some minor issues should be resolved for Wednesday.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 13 Dec 2016 15:53 UTC Replying to [comment:56 hansolsson]:

Reviewed changes. Documents is updated, but some minor issues should be resolved for Wednesday.

Preliminarily updated in r9569: https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges_POe.doc

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 14 Dec 2016 11:16 UTC Unclear issues:

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 14 Dec 2016 12:40 UTC Formulation of validity of conditional components:

  1. Not added: At each visited component instance, the name is inserted into the variables list. Then the conditional declaration expression is evaluated if applicable. If the expression cannot be evaluated yet (it involves variables that have not been handled) this instance is handled after the non-conditional components. If there is no such expression or it evaluates to true, following is done with the instance:

  2. Removed: At each visited component instance, the name is inserted into the variables list. Conditional components with false condition are removed later and they are not part of the simulation model. [Thus e.g. parameters don't need values in them. However, type-error can be detected.]

It seems the result is the same - but the texts differ. Note: the component is already instantiated. Poll for formulation:

  1. 1
  2. 5 (selected)
modelica-trac-importer commented 6 years ago

Comment by hansolsson on 14 Dec 2016 14:25 UTC Updated after discussion - clarifying the behavior for functions etc in r9583

https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges.doc

Decision: Put into "Under evaluation" on Thursday 2016-12-15, or if there are unclear issues resolve and put "Under evaluation" on Friday 2016-12-16. Agreement by acclamation.

modelica-trac-importer commented 6 years ago

Comment by kurzbach on 15 Dec 2016 11:42 UTC There is stiil a minor thing: in 5.6.2 there is the sentence starting with "The found object is checked, ..."

This has to be moved (with some reformulation) after the (new) sentence starting with "Conditional components with false ..."

to e.g.: "References are checked, whether they are still part of the simulation model."

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 15 Dec 2016 13:47 UTC Replying to [comment:61 kurzbach]:

There is stiil a minor thing: in 5.6.2 there is the sentence starting with "The found object is checked, ..."

This has to be moved (with some reformulation) after the (new) sentence starting with "Conditional components with false ..."

to e.g.: "References are checked, whether they are still part of the simulation model."

This should be resolved in r9586

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 16 Dec 2016 15:30 UTC As decided on design meeting - after some possible minor updates (have been done). Put these two MCP under Evaluation today (2106-12-16).

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 19 Jan 2017 10:37 UTC This poll should have been started a week ago according to plan, but better late than never. This MCP is currently under Evaluation - and have been for a month, and we can thus decide to Accept it (and also add to Modelica Specification 3.4).

[[Poll(Result of evaluation of MCP - closing Friday January 27;Accept and add to 3.4.;Accept - without adding to 3.4.;Do not accept.)]]

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 19 Jan 2017 10:46 UTC Replying to [comment:64 hansolsson]:

This poll should have been started a week ago according to plan, but better late than never. This MCP is currently under Evaluation - and have been for a month, and we can thus decide to Accept it (and also add to Modelica Specification 3.4).

[[Poll(Result of evaluation of MCP - closing Friday January 27;Accept and add to 3.4.;Accept - without adding to 3.4.;Do not accept.)]]

Seems that this didn't work (; if someone can indicate how to change it today before 15.00 - do so; otherwise I will have to use a backup like doodle poll.

modelica-trac-importer commented 6 years ago

Comment by jmattsson on 19 Jan 2017 12:01 UTC That plugin requires additional permissions. From TracHacks: The following permissions are added with this plugin:

The POLL_VOTE permission allows users to vote.
The POLL_VIEW permissions allows users to view the poll's result.

So it should work if you can get Martin S to add POLL_VIEW to everyone and POLL_VOTE to MA members - there should already be groups that they can be added on.

modelica-trac-importer commented 6 years ago

Comment by sjoelund.se on 23 Jan 2017 14:00 UTC It's unclear to me what we are voting on. The public folder under SVN is empty, so it cannot be an MCP under evaluation. There should be a PDF/HTML document that we can read to see what the changes are. The MAinternal directory contains many different versions of the text, which confuses things even more

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 23 Jan 2017 14:17 UTC Replying to [comment:67 sjoelund.se]:

It's unclear to me what we are voting on. The public folder under SVN is empty, so it cannot be an MCP under evaluation.

An MCP need only be public after "Under Evaluation" if accepted - see graph in https://svn.modelica.org/projects/MCP/public/ModelicaSpecification_DevelopmentProcess.pdf

(Another of the tickets is also hidden; I remember that I checked this.)

There should be a PDF/HTML document that we can read to see what the changes are. The MAinternal directory contains many different versions of the text, which confuses things even more

I will try to check this as soon as possible.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 23 Jan 2017 14:36 UTC Replying to [comment:68 hansolsson]:

Replying to [comment:67 sjoelund.se]:

It's unclear to me what we are voting on. The public folder under SVN is empty, so it cannot be an MCP under evaluation.

An MCP need only be public after "Under Evaluation" if accepted - see graph in https://svn.modelica.org/projects/MCP/public/ModelicaSpecification_DevelopmentProcess.pdf

(Another of the tickets is also hidden; I remember that I checked this.)

There should be a PDF/HTML document that we can read to see what the changes are. The MAinternal directory contains many different versions of the text, which confuses things even more

I will try to check this as soon as possible.

It's the version without suffixes that is the relevant one:

https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges.doc

(and https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_Overview.docx is still a motivation - but very high-level.)

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 23 Jan 2017 15:35 UTC For non-Microsoft persons I added ​https://svn.modelica.org/projects/MCP/MAinternal/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges.pdf

(corresponding to the "Final" version of the doc-file).

modelica-trac-importer commented 6 years ago

Comment by eshmoylova on 23 Jan 2017 21:14 UTC The proposal looks really good after the revisions. I just have a couple of minor comments regarding the proposed changes.

  1. Simple name lookup. In the current specification there is s short sentence saying what a simple name is. Shouldn't it be kept?

  2. Global name lookup. There is no section on global name lookup in the proposed changes. Does it mean that it is proposed to remove this section from the specification?

  3. Section 5.6.1.4, The element itself. It says, "The class of a partially instantiated component is found in the instance tree...." What is not clear is what happens in the following case.

    model M
B b;
extends C;
...
end M;

    where class B is defined in class C. When we build the instance tree for M, do we find a class for each component as we process it? Will we know that there is a class B inside M when we are looking for a class of b.

  4. Section 5.6.2. There is a term "top-level variables" used in this section. It is not clear from the context what is meant by it, and there is no explicit definition given. What are the top-level variables?

modelica-trac-importer commented 6 years ago

Comment by anonymous on 24 Jan 2017 15:30 UTC Replying to [comment:71 eshmoylova]:

  1. Simple name lookup. In the current specification there is s short sentence saying what a simple name is. Shouldn't it be kept?

Sure, this should be kept.

  1. Global name lookup. There is no section on global name lookup in the proposed changes. Does it mean that it is proposed to remove this section from the specification?

No. This is only by accident. The focus of the work was put on section 5.6 mostly. So the other sections of chapter 5 may still need some effort.

  1. Section 5.6.1.4, The element itself. It says, "The class of a partially instantiated component is found in the instance tree...." What is not clear is what happens in the following case. 
    model M
B b;
extends C;
...
end M;

    where class B is defined in class C. When we build the instance tree for M, do we find a class for each component as we process it? Will we know that there is a class B inside M when we are looking for a class of b.

The heading "The element itself" means in this case the instance of model M. So there is no need to find the class B yet. The classes from extends clauses are partially instantiated in the third step "The inherited contents of the element", just before in the fourth step the components are instantiated, where B needs to be found.

  1. Section 5.6.2. There is a term "top-level variables" used in this section. It is not clear from the context what is meant by it, and there is no explicit definition given. What are the top-level variables?

You are right, this is a little bit unclear and should be improved.

modelica-trac-importer commented 6 years ago

Comment by jmattsson on 24 Jan 2017 15:41 UTC So, if I understand the MCP process correctly, those issues can't be addressed unless we reject the MCP in the current voting and move it back to "under development". Is that correct?

modelica-trac-importer commented 6 years ago

Comment by kurzbach on 24 Jan 2017 16:00 UTC [comment:72 comment:72] was from me (kurzbach). Accidently I wasn't logged in. Sorry.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 24 Jan 2017 16:09 UTC Replying to [comment:73 jmattsson]:

So, if I understand the MCP process correctly, those issues can't be addressed unless we reject the MCP in the current voting and move it back to "under development". Is that correct?

I agree that it is a good question for the MCP process.

A pragmatic solution is to accept it, and then correct these issues during the alpha/beta/release candidate process for Modelica 3.4 - since they are non-blocking and can be corrected in that later stage (as far as I understand) - similarly as if we had found them later. What we clearly don't want is that people delay reporting such issues until it is 'Accepted' to ensure acceptance.

The MCP process should be updated by stating such minor issues can be reported.

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 30 Jan 2017 08:56 UTC The vote on this is now completed: 11 Accept and add to 3.4 0 Accept without adding to 3.4 0 Do not accept.

Will move the finished proposal to public part as well:

https://svn.modelica.org/projects/MCP/public/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges.doc

https://svn.modelica.org/projects/MCP/public/MCP-0019_Flattening/MCP_0019_Flattening_SpecChanges.pdf

https://svn.modelica.org/projects/MCP/public/MCP-0019_Flattening/MCP_0019_Flattening_Overview.docx

modelica-trac-importer commented 6 years ago

Modified by hansolsson on 30 Jan 2017 12:30 UTC

modelica-trac-importer commented 6 years ago

Comment by hansolsson on 7 Feb 2017 11:43 UTC Added (including the changes proposed above) in r9626 (including keeping description of simple name and describing top-level variable for functions). It might still need polishing.