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

Implement Base Modelica output #11820

Open
3 of 6 tasks
casella opened this issue Jan 17, 2024 · 17 comments
Open
3 of 6 tasks

Implement Base Modelica output #11820

casella opened this issue Jan 17, 2024 · 17 comments
Assignees
Labels
COMP/OMC/Frontend Issue and pull request related to the frontend enhancement
Milestone

Comments

@casella
Copy link
Contributor

casella commented Jan 17, 2024

The new Base Modelica standard, presented in this paper at the Aachen Modelica Conference, is currently in draft form within MCP0031.

OpenModelica has supported flat Modelica output for quite some time, via the -f compiler flag. During the last two years, we used this feature to provide a frontend to the MARCO compiler (see paper), adding some more flags that produce an output that is very close to what is required by Base Modelica. Currently, there are two possible setups:

  1. Flatting to scalars: -f -d=printRecordTypes --showStructuralAnnotations
  2. Flattening with array preserving: -f -d=nonfScalarize,arrayConnect,combineSubscripts,printRecordTypes,vectorizeBindings --showStructuralAnnotations

Setup 2. is particularly interesting for large models containing huge arrays, as it provides a compact representation that can be used for efficient code generation.

We should finalize BaseModelica output with the following actions:

  • implement a --baseModelica compiler flag, with optional arguments such as --baseModelica=preserveArrays, to provide the same effect as the flag combinations reported above
  • enclose the flat model in a package with the same name
  • suppress public/private keywords in functions and models
  • (optional) avoid constant-evaluating constants to literals, keep them as explicit package constants, even though the code obtained by constant-evaluating constants is still perfectly legal Base Modelica
  • make sure that attributes of record member variables are output correctly, see Wrong flat Modelica output of records with start attributes #11792
  • test the BaseModelica export with a number of cases and ensure we get the correct output, based on inspection

This would give us a first fairly complete implementation of Base Modelica, available as open-source software.

@casella casella added enhancement COMP/OMC/Frontend Issue and pull request related to the frontend labels Jan 17, 2024
@casella casella added this to the 1.23.0 milestone Jan 17, 2024
@casella
Copy link
Contributor Author

casella commented Jan 17, 2024

Adding @mtiller, @mscuttari, @fedetft, @danielecattaneo, @adrpo to the loop.

@casella
Copy link
Contributor Author

casella commented Jan 22, 2024

We could also at some point implement an online service that gets a .mo file and the name of the class that must be flattened, and returns its BaseModelica output. As long as the code is not confidential and it only depends on open source libraries that are referenced by the uses annotation and included in our Package Manager list, it should work out of the box.

@perost
Copy link
Member

perost commented Jan 22, 2024

#11853 encloses everything in a package with the same name as the model. I also moved function and record declarations to outside the model, previously everything was contained in the model but with the package there's no need for that.

I also noticed two issues:

  • public/protected is printed out even though Base Modelica doesn't have that. Removing it from the output is trivial but breaks some of our test cases, because the compiler does not have a Base Modelica mode when compiling a model (so it doesn't allow e.g. public non-inputs/outputs in functions).
  • We currently use the full path of the model as the name, but I think it's supposed to just be the last part (i.e. 'Model' rather than 'Library.Package.Model').

@casella
Copy link
Contributor Author

casella commented Jan 23, 2024

#11853 encloses everything in a package with the same name as the model. I also moved function and record declarations to outside the model, previously everything was contained in the model but with the package there's no need for that.

Sounds good. I though it would be the same, but it isn't, because a function inside a model can still access components of the model via lookup, right?

* public/protected is printed out even though Base Modelica doesn't have that.

Are you sure? There is an example in the paper at page 6 with protected variable declarations in functions.

Removing it from the output is trivial but breaks some of our test cases, because the compiler does not have a Base Modelica mode when compiling a model (so it doesn't allow e.g. public non-inputs/outputs in functions).

Maybe it's not necessary to do so. We need to check further

* We currently use the full path of the model as the name, but I think it's supposed to just be the last part (i.e. `'Model'` rather than `'Library.Package.Model'`).

Why should it? The Base Modelica does not describe how you lower a Modelica model into Base Modelica, it only describes the grammar and semantics of Base Modelica. As I understand, any legal Modelica classname would be ok, including the quoted full path

@casella
Copy link
Contributor Author

casella commented Jan 23, 2024

I guess we should also keep a list of known issue with our implementation at the beginning of this ticket, and keep it up to date.

@perost
Copy link
Member

perost commented Jan 23, 2024

  • public/protected is printed out even though Base Modelica doesn't have that.

Are you sure? There is an example in the paper at page 6 with protected variable declarations in functions.

It says so here, see section "Protected" towards the end of the page. public/protected is also not part of the Base Modelica grammar.

* We currently use the full path of the model as the name, but I think it's supposed to just be the last part (i.e. `'Model'` rather than `'Library.Package.Model'`).

Why should it? The Base Modelica does not describe how you lower a Modelica model into Base Modelica, it only describes the grammar and semantics of Base Modelica. As I understand, any legal Modelica classname would be ok, including the quoted full path

I didn't see any examples using the fully qualified path, but I guess there's also nothing saying you can't use it. It just seemed a bit weird having to write:

translateModel('Modelica.Mechanics.MultiBody.Examples.Elementary.DoublePendulum'.'Modelica.Mechanics.MultiBody.Examples.Elementary.DoublePendulum', fileNamePrefix="DP");

But I guess I'll just leave it as it is for now.

@casella
Copy link
Contributor Author

casella commented Jan 24, 2024

It says so here, see section "Protected" towards the end of the page.

True. I wrote a comment on the the PR that introduced that part, maybe we get some feedback about it. I guess it was an oversight.

public/protected is also not part of the Base Modelica grammar.

Yes. And the relevant section of the proposal states clearly that it's not needed in function declarations.

So, we just remove protected when generating --baseModelica output and that's it.

@casella
Copy link
Contributor Author

casella commented Jan 24, 2024

I didn't see any examples using the fully qualified path, but I guess there's also nothing saying you can't use it. It just seemed a bit weird having to write:

translateModel('Modelica.Mechanics.MultiBody.Examples.Elementary.DoublePendulum'.'Modelica.Mechanics.MultiBody.Examples.Elementary.DoublePendulum', fileNamePrefix="DP");

It absolutely is, of course 😄 . I hadn't thought about that. Please just use the class name, not the fully qualified one. At the end of the day, the name could also be "X" (Elon would love it), since there is only one package and one model in a Base Modelica model.

@perost
Copy link
Member

perost commented Jan 25, 2024

#11874 changes it to use the class name instead of the fully qualified path.

Additionally, #11875 renames the existing --flatModelica (-f) to --baseModelica. Maybe we should also change the short name to -b instead? We already have a -b flag, but it should be removed anyway since it was used for the modpar library that we removed a decade ago.

Also, in your example you used -d=printRecordTypes --showStructuralAnnotations, but those flags only changes the normal flat output and not the Base Modelica one. So it's only the array preserving case that might need additional flags.

@casella
Copy link
Contributor Author

casella commented Jan 25, 2024

@perost I am not sure we should remove flags so lightly or, even worse, keep them with a different meaning. People use them, and their code will be broken as a result. I would suggest to maybe deprecate --flatModelica but still keep it around for some time, and avoid re-using -b for that.

Also, in your example you used -d=printRecordTypes --showStructuralAnnotations, but those flags only changes the normal flat output and not the Base Modelica one. So it's only the array preserving case that might need additional flags.

Sorry, which example do you refer to?

@perost
Copy link
Member

perost commented Jan 25, 2024

@perost I am not sure we should remove flags so lightly or, even worse, keep them with a different meaning. People use them, and their code will be broken as a result. I would suggest to maybe deprecate --flatModelica but still keep it around for some time, and avoid re-using -b for that.

As far as I know no one is actually using --flatModelica, only the short form -f and you and your colleagues are the only ones using that one. So I don't think renaming --flatModelica to --baseModelica will cause any issues.

As for changing it to -b that's a different matter since some people as mentioned are using -f, so I haven't changed that. But I suspect that if we don't change it now we never will. But I've removed the old -b flag at least, it hasn't done anything for more than 10 years and all the flags to enable the features that used it are long gone.

Also, in your example you used -d=printRecordTypes --showStructuralAnnotations, but those flags only changes the normal flat output and not the Base Modelica one. So it's only the array preserving case that might need additional flags.

Sorry, which example do you refer to?

You gave two cases, one scalar and one with arrays, in the issue description.

@casella
Copy link
Contributor Author

casella commented Jan 25, 2024

As far as I know no one is actually using --flatModelica, only the short form -f and you and your colleagues are the only ones using that one. So I don't think renaming --flatModelica to --baseModelica will cause any issues.

Could be, but we don't really know what people are doing out there. No reason to make them angry.

As for changing it to -b that's a different matter since some people as mentioned are using -f, so I haven't changed that. But I suspect that if we don't change it now we never will. But I've removed the old -b flag at least, it hasn't done anything for more than 10 years and all the flags to enable the features that used it are long gone.

I would stick to --BaseModelica, which is clear and unambiguous. -b may mean 1000 things. People used short flags when they had decks of cards for input, but those days are long past.

Also, in your example you used -d=printRecordTypes --showStructuralAnnotations, but those flags only changes the normal flat output and not the Base Modelica one. So it's only the array preserving case that might need additional flags.

Agreed. That was also my idea. As to array preserving, at the moment we have two variants: preserve arrays and collect classes into arrays.

@perost
Copy link
Member

perost commented Jan 25, 2024

As far as I know no one is actually using --flatModelica, only the short form -f and you and your colleagues are the only ones using that one. So I don't think renaming --flatModelica to --baseModelica will cause any issues.

Could be, but we don't really know what people are doing out there. No reason to make them angry.

Sure, but we're talking about a flag whose description makes it clear that it's experimental, and we've changed the output numerous times since adding it without anyone saying a word.

I would stick to --BaseModelica, which is clear and unambiguous. -b may mean 1000 things. People used short flags when they had decks of cards for input, but those days are long past.

Everyone currently known to use the flag uses the short flag though, I didn't even remember it had a long form myself until I checked the flag definition. The issue is just that the short form of --baseModelica is -f, which makes little sense unless you know why that is. But that's fairly common, there are only so many possible short flags after all, so I guess it's not really a big deal.

@casella
Copy link
Contributor Author

casella commented Jan 25, 2024

Sure, but we're talking about a flag whose description makes it clear that it's experimental, and we've changed the output numerous times since adding it without anyone saying a word.

Fair enough 😃

I would stick to --BaseModelica, which is clear and unambiguous. -b may mean 1000 things. People used short flags when they had decks of cards for input, but those days are long past.

Everyone currently known to use the flag uses the short flag though, I didn't even remember it had a long form myself until I checked the flag definition. The issue is just that the short form of --baseModelica is -f, which makes little sense unless you know why that is. But that's fairly common, there are only so many possible short flags after all, so I guess it's not really a big deal.

Go for -f. In 3256 A.D. historians will debate why we used that letter 😄

@casella
Copy link
Contributor Author

casella commented Feb 1, 2024

@perost I prepared a small demo I'd like to show around. It contains all the basic ingredients: models, functions, and record.
BaseModelicaDemo.mo.txt.

The --BaseModelica output seems fine for me, except for the protected keyword in the function declaration. Can you please suppress that, as we agreed here?

Thanks!

@perost
Copy link
Member

perost commented Feb 2, 2024

@perost I prepared a small demo I'd like to show around. It contains all the basic ingredients: models, functions, and record. BaseModelicaDemo.mo.txt.

The --BaseModelica output seems fine for me, except for the protected keyword in the function declaration. Can you please suppress that, as we agreed here?

Thanks!

Fixed in #11931. I also changed the frontend to not check that non-input/output parameters in functions are protected when --baseModelica is used to avoid breaking our test cases.

It's maybe not a good idea to have the same flag for compiling Base Modelica as outputting Base Modelica, it should probably be based on e.g. file extension instead to be more flexible. But until that's standardised I guess this is fine.

@casella
Copy link
Contributor Author

casella commented Feb 2, 2024

It's maybe not a good idea to have the same flag for compiling Base Modelica as outputting Base Modelica, it should probably be based on e.g. file extension instead to be more flexible. But until that's standardised I guess this is fine.

--baseModelica is only meant to produce Base Modelica output.

For input, MCP 0031 defines a version header, e.g. //! base 1.0.0. OMC should look for that in order to decide if a .mo source code file is BaseModelica and not Modelica.

@perost I guess you can implement that already, maybe you can ignore the version number for the time being, as it is still not well defined.

@casella casella modified the milestones: 1.23.0, 1.24.0 Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP/OMC/Frontend Issue and pull request related to the frontend enhancement
Projects
None yet
Development

No branches or pull requests

2 participants