ModelicaSpecification icon indicating copy to clipboard operation
ModelicaSpecification copied to clipboard

Apply modern table presentation to some more annotations

Open henrikt-ma opened this issue 3 years ago • 34 comments

What started with the intention of just fixing a minor formatting issue related to missingInngerMessage ended up as a wider sweep of modernizing the presentation of annotations using tables. With this, all annotations presented outside the Annotations chapter use the modern style. In particular, all annotations in the Functions chapter are now presented in the same way.

Ironically, missingInngerMessage still belongs to one of the annotation sections where the presentation hasn't been modernized. It is one of the two more obvious sections next in turn for modernization (possibly added to this PR to avoid merge conflicts, unless this PR is closed quickly):

  • [ ] 18.7 Annotations for the Graphical User Interface (presentation would benefit enormously from new style)
  • [x] 18.8 Annotations for Version Handling (can be done with very little work)

Then there are also the annotations presented using more or less deeply nested structures of pseudo code records, and these are not as clear how to modernize. Examples include:

  • 18.2.2 Annotations for Figures
  • 18.9 Annotations for Access Control to Protect Intellectual Property

henrikt-ma avatar Jun 17 '21 20:06 henrikt-ma

With the merge conflicts now resolved, maybe this would be a good time to get this merged?

henrikt-ma avatar Sep 02 '21 11:09 henrikt-ma

@HansOlsson don't you think merging this could be considered a nice step in the direction for annotation cleanup decided at the last web meeting?

henrikt-ma avatar Nov 08 '21 21:11 henrikt-ma

@HansOlsson don't you think merging this could be considered a nice step in the direction for annotation cleanup decided at the last web meeting?

Possibly, but I saw that only 1 of 2 tasks were completed, and thus skipped it.

HansOlsson avatar Nov 09 '21 15:11 HansOlsson

@HansOlsson, would it help moving this forward if I request reviews from other people? I feel we are making progress on the presentation on so many annotations now, and it would be nice if we could move on to the principal discussion about what to do with things like the deeply nested pseudo-code record structures and the unstructured annotations that can't be presented as pseudo-code records.

henrikt-ma avatar Dec 11 '21 21:12 henrikt-ma

Merge conflicts have been resolved, so this is once again ready. Adding another reviewer, hoping for more action.

henrikt-ma avatar Jan 28 '22 06:01 henrikt-ma

@HansOlsson, would it help moving this forward if I request reviews from other people? I feel we are making progress on the presentation on so many annotations now, and it would be nice if we could move on to the principal discussion about what to do with things like the deeply nested pseudo-code record structures and the unstructured annotations that can't be presented as pseudo-code records.

Possibly, but:

  • I don't think we should think of it as pseudo-code records, but as pseudo-code declarations. Thus singleInstance can still be defined as a pseudo-code declaration (Boolean singleInstance=false), similarly Boolean choicesAllMatching.
  • Even Include could be kind of a pseudo-declaration, StringScalarOrVector Include.
  • And I think we should clearly prefer pseudo-code declarations; as the grammar fragments have caused a number of problems.

HansOlsson avatar Jan 28 '22 14:01 HansOlsson

I see some of the style 3 converted into style 2 by this PR but not all. Is that ok or do we want to find all instances?

Also, it seems to me that the decision was to convert more to the record style.

To me the record style is preferable.

Of course, that would apply only to structured annotations. For consistency, it would make sense to present simple annotations as component declarations:

String defaultComponentName = "name";

But it is harder with cases where only couple of values are allowed. For example, preferredView can only be "info", "diagram", "icon" and "text". You would need to say something like:

String preferredView = view;

where view is one of "info", diagram", "icon", and "text".

Or we could use another annotation to describe that:

parameter String preferredView 
  annotation(choices(
       choice="info" "Documentation view", 
       choice="diagram" "Graphical diagram", 
       choice="icon" "Icon of model",
       choice="text" "Modelica source code"));

I understand it looks close to recursive, but it might still be useful.

HansOlsson avatar Feb 09 '22 13:02 HansOlsson

Or we could use another annotation to describe that:

parameter String preferredView 
  annotation(choices(
       choice="info" "Documentation view", 
       choice="diagram" "Graphical diagram", 
       choice="icon" "Icon of model",
       choice="text" "Modelica source code"));

I understand it looks close to recursive, but it might still be useful.

I thought about using choices. What I am not sure about is: does it indicate that no other values are allowed? The description of choices only says that each choice indicates a suitable redeclaration or modification. But are those suitable values the only ones that are allowed? Maybe it does't matter. Probably if the preferredView is something other that the ones above it would be ignored. Or is it supposed to give an error?

eshmoylova avatar Feb 09 '22 14:02 eshmoylova

I thought about using choices. What I am not sure about is: does it indicate that no other values are allowed? The description of choices only says that each choice indicates a suitable redeclaration or modification. But are those suitable values the only ones that are allowed? Maybe it does't matter. Probably if the preferredView is something other that the ones above it would be ignored. Or is it supposed to give an error?

I would say it doesn't matter much. I could imagine that we later want to extend this (similarly as "icon" was added); so we shouldn't make it a hard error.

HansOlsson avatar Feb 09 '22 14:02 HansOlsson

I see some of the style 3 converted into style 2 by this PR but not all. Is that ok or do we want to find all instances?

My intention with this PR is only to modernize the presentation of some more annotations, not to fix the global problem of getting all annotations into the new form, whatever that is.

However, after this PR, I think it would be the right time to take a step back and think about what styles we should really use in the end. Some questions to think about are mentioned in the initial comment of this PR. (With the recent introduction of the section on notation in the introduction, we now have a perfect place for explaining how the different variants are to be interpreted, and we should reference these explanations from the beginning of the Annotations chapter.)

henrikt-ma avatar Feb 10 '22 22:02 henrikt-ma

I thought about using choices. What I am not sure about is: does it indicate that no other values are allowed? The description of choices only says that each choice indicates a suitable redeclaration or modification. But are those suitable values the only ones that are allowed? Maybe it does't matter. Probably if the preferredView is something other that the ones above it would be ignored. Or is it supposed to give an error?

I would say it doesn't matter much. I could imagine that we later want to extend this (similarly as "icon" was added); so we shouldn't make it a hard error.

Didn't we just spend quite a lot of time to come up with the current design where the variable view is used in the "structured presentation" of the annotation, with a description of the allowed values for view given in the text below? I don't think it's that bad, and at least less strange compared to explaining annotations in terms of choices. On the other hand, I like tables, so I think expressing the the possible values for view using a table instead would be a nicely structured way of presenting it all.

henrikt-ma avatar Feb 10 '22 22:02 henrikt-ma

I see some of the style 3 converted into style 2 by this PR but not all. Is that ok or do we want to find all instances?

My intention with this PR is only to modernize the presentation of some more annotations, not to fix the global problem of getting all annotations into the new form, whatever that is.

To me that seems odd.

I would first define what style we prefer as the modern one (as stated above I prefer the "record-style" regardless of whether it is records, scalars, arrays or pseudo-records), and then try to work to get all (or most) annotations to use that.

It might also be that this PR is trying to do two things (which is always complicated):

  • Present how the annotation works in a new way
  • Present the syntax/grammar/what-ever for the annotation in a new way

And we focus on different parts of this, you might be looking at the first part and the rest of us are stuck on the second part.

HansOlsson avatar Feb 11 '22 10:02 HansOlsson

I see some of the style 3 converted into style 2 by this PR but not all. Is that ok or do we want to find all instances?

My intention with this PR is only to modernize the presentation of some more annotations, not to fix the global problem of getting all annotations into the new form, whatever that is.

To me that seems odd.

Maybe someone got the impression that this PR is partly introducing new styles in order to partly solve the annotation cleanup we decided to do long time ago. That's not the case. This PR only does what can be done easily using the modern style for annotations that is already in place.

I would first define what style we prefer as the modern one (as stated above I prefer the "record-style" regardless of whether it is records, scalars, arrays or pseudo-records), and then try to work to get all (or most) annotations to use that.

The point was to do away with the easy parts, so that only the parts needing more discussion would need to wait. By this split, it also gets easier for us to filter out the annotations for which the current "modern style" is insufficient.

It might also be that this PR is trying to do two things (which is always complicated):

  • Present how the annotation works in a new way
  • Present the syntax/grammar/what-ever for the annotation in a new way

And we focus on different parts of this, you might be looking at the first part and the rest of us are stuck on the second part.

No, I don't think that's the case, but I have the impression that it is the fear of this being the case that's holding it back.

henrikt-ma avatar Feb 14 '22 07:02 henrikt-ma

The point was to do away with the easy parts, so that only the parts needing more discussion would need to wait. By this split, it also gets easier for us to filter out the annotations for which the current "modern style" is insufficient.

Before anything else I believe we should clarify what we are discussing with 'modern' presentation. That ambiguity is a general problem with adjectives like 'modern', 'new' etc.

  • Is it a new way of presenting annotations in terms of new latex-commands.
  • Or is it a different way of presenting the syntax/grammar/what-ever for the annotation?

HansOlsson avatar Feb 14 '22 14:02 HansOlsson

I am waiting on agreement on what is considered "modern" (as well as not having much time to devote to it at the moment).

eshmoylova avatar Feb 14 '22 14:02 eshmoylova

The point was to do away with the easy parts, so that only the parts needing more discussion would need to wait. By this split, it also gets easier for us to filter out the annotations for which the current "modern style" is insufficient.

Before anything else I believe we should clarify what we are discussing with 'modern' presentation. That ambiguity is a general problem with adjectives like 'modern', 'new' etc.

Sure.

  • Is it a new way of presenting annotations in terms of new latex-commands.

No.

  • Or is it a different way of presenting the syntax/grammar/what-ever for the annotation?

Yes, and this style was introduced long time ago (buy now). It was probably simply too much work or risk of introducing merge conflicts with other ongoing changes for it to get immediately applied widely. This PR does a lot of that work, and allows us to see more clearly what remains to design.

I don't see any major problems with extending the design to also work in the remaining cases, but I think it would help us if we could separate the application of a style that has already been introduced from introducing new design that can also handle the remaining annotations.

henrikt-ma avatar Feb 15 '22 22:02 henrikt-ma

Yes, and this style was introduced long time ago (buy now). It was probably simply too much work or risk of introducing merge conflicts with other ongoing changes for it to get immediately applied widely. This PR does a lot of that work, and allows us to see more clearly what remains to design.

I don't recall this design choice, and I think it makes sense to revisit it after seeing the consequences.

The basic problem is that by presenting it as a grammar we get something that looks neither as the Modelica code you would write nor as the declaration of the relevant items, and things get lost in the syntax.

The styles we currently have are:

  • Declared in Modelica as used in https://specification.modelica.org/master/annotations.html#annotations-for-figures and https://specification.modelica.org/master/annotations.html#graphical-primitives https://specification.modelica.org/master/annotations.html#access-control-to-protect-intellectual-property which is my preferred variant. It was also the basis for the custom annotations proposal.
  • Correct grammar as in https://specification.modelica.org/master/annotations.html#annotations-for-simulations which to me hides the relevant parts in syntax and should be replaced by declarations in Modelica. A key observation is that we previously only considered records, but we also need annotations of simple types as well.
  • Something between example code and grammar as in https://specification.modelica.org/master/annotations.html#annotations-for-version-handling which is complicated for a number of reasons. Just changing to a grammar will not fix that, there are other tweaks needed as well.

The reason we used a mix of example code and grammar is that it's easier to read choices than "choices", when referring to choices-annotation (and similarly in other cases).

The reasons I prefer the first variant is actually two-fold:

  • It seems the easiest to understand so far; and an easier design reduces the risk of errors.
  • It constrains our thinking for future annotations. Instead of considering designing a grammar/language for something new we will be looking at designing its data (or data-structure). I know that we cannot get all annotations in that form, but the more the better.

The second point has an interesting implication for one of the discussion points - IncludeDirectory which currently can be a string or a string-vector. If we start thinking of data-structures it would make sense that IncludeDirectory is a string-vector, with some backward compatibility fix for a scalar string. Such observations are valuable and comes from restricting the choices to a data-structure.

HansOlsson avatar Feb 22 '22 09:02 HansOlsson

I am with @HansOlsson. I do not believe the grammar style is a good way to present the annotations. It is not clear to me that there was a group decision to use the grammar style as the preferred style. From the discussions that I remember the preference was given to the record style. There was a hesitancy because we can only use records for structured types. But as we already discussed in this PR it can be accomplished by using types. Perhaps we should call it type-record style instead of just record type. So I do not think we should spend time on converting anything into the grammar style as done in this PR, and my preference is to close this PR.

@henrikt-ma if you feel strongly that there is reason to use the grammar style for annotations then I suggest we leave this open and bring it up for discussion at the next meeting.

eshmoylova avatar Mar 04 '22 14:03 eshmoylova

Just replying quickly before going on vacation for one week. The goal of the PR is not to favor the use of grammar over pseudo-code, the goal is to transform the presentation into the style where we have tables summarizing the annotation described in a section, plus a structured way of framing the presentation of each annotation.

The use of grammar is only a consequence of not being able to directly apply the pseudo-code style that we have successfully applied to some of our other annotations. In other words, I'm trying to do the best of what we have today. If we had better alternatives available I'd happily apply them.

For me, it would be OK to work out those better alternatives as part of this PR, although my preference would be to have a separate discussion about this later, and I think it will be easier to have that discussion if we can see clearly which annotations that currently need grammar in order to describe their structure with sufficient precision.

I'll be back on March 14, take care until then!

henrikt-ma avatar Mar 04 '22 21:03 henrikt-ma

Regarding how to write choicesAllMatching I prefer the simple Boolean choicesAllMatching. Partially because it is simple, and partially because there are various enhancements ideas of supporting non-literal values for such parameters; and we shouldn't just rule them out immediately. One is using parameter Boolean p=true;parameter Real x=2 annotation(Evaluate=p); (added due to demand in Dymola, I believe there might already be an issue proposing that, but I couldn't find it) another is #3133

Update: clarified "Issue".

HansOlsson avatar Mar 18 '22 15:03 HansOlsson

Regarding how to write choicesAllMatching I prefer the simple Boolean choicesAllMatching. Partially because it is simple, and partially because there are various enhancements ideas of supporting non-literal values for such parameters; and we shouldn't just rule them out immediately. One is using parameter Boolean p=true;parameter Real x=2 annotation(Evaluate=p); (added due to demand in Dymola, I believe there might already be an issue proposing that, but I couldn't find it) another is #3133

Update: clarified "Issue".

Changing from literal values to Boolean parameter or constant would be a welcome change, but I don't think we should mix changes with cleanup of the presentation. Either a change like this should be discussed as a separate issue, or we should have a bigger MCP level discussion about general relaxation of annotation expressions.

Having a cleaned up presentation of where we are today will be the best starting point for future discussions about how to change semantics.

henrikt-ma avatar Mar 20 '22 22:03 henrikt-ma

Regarding how to write choicesAllMatching I prefer the simple Boolean choicesAllMatching. Partially because it is simple, and partially because there are various enhancements ideas of supporting non-literal values for such parameters; and we shouldn't just rule them out immediately. One is using parameter Boolean p=true;parameter Real x=2 annotation(Evaluate=p); (added due to demand in Dymola, I believe there might already be an issue proposing that, but I couldn't find it) another is #3133 Update: clarified "Issue".

Changing from literal values to Boolean parameter or constant would be a welcome change, but I don't think we should mix changes with cleanup of the presentation.

True, and the problem with this PR is exactly that it is perceived as changing more than one thing - and the intended changes are lost among the other ones.

HansOlsson avatar Mar 28 '22 07:03 HansOlsson

@henrikt-ma, @HansOlsson, @sjoelund, I would like to view the build for this PR, so that I don't have to build it myself. I used to be able to access the automatic builds on https://test.openmodelica.org/jenkins/job/ModelicaAssociation/job/ModelicaSpecification/view/change-requests/

Now when I click on the link it asks sign in and no option to sign up or forgot password or anything. Am I allowed to get access to that and, if yes, how?

eshmoylova avatar Mar 31 '22 15:03 eshmoylova

@sjoelund have you changed anything?

HansOlsson avatar Mar 31 '22 15:03 HansOlsson

@henrikt-ma, @HansOlsson, @sjoelund, I would like to view the build for this PR, so that I don't have to build it myself.

While waiting for CI builds to become available again, I'll send the document directly to @eshmoylova.

henrikt-ma avatar Mar 31 '22 15:03 henrikt-ma

Perhaps it would be better to rename this PR into "Modernize table presentation of some more annotation"?

I did a small variation of that.

henrikt-ma avatar Mar 31 '22 19:03 henrikt-ma

One more thought about the format of type/record vs grammar before I forget it: how would you represent the choice annotation in a record format? It is supposed to be any possible modification on the class for which it is specified, so the structure of the annotation is the same as the corresponding class.

I don't know. Some annotations will be easier than others to express without the use of grammar. I think choices will be one of the hardest. If we do the others first, maybe we find some patterns that can be applied to choices as well… if not I'd be fine to stick with grammar for the exceptional cases where the record format doesn't make sense at all. If we do that, it would probably be a good idea to not only use a listing with the "grammar" language, but to also write with text that this annotation is defined using grammar rather than "record style".

henrikt-ma avatar Mar 31 '22 20:03 henrikt-ma

Looking more I don't think that using textvisiblespace is significantly better than just plain space. If we want to make it clearer we could use Modelica for something like \lstineline!"$packageName$"+" "+"$packageVersion$"+".mo"!

HansOlsson avatar Apr 01 '22 12:04 HansOlsson

The title of the PR looks good. But I wonder if the description should be updated a bit as well. Maybe just adding updating "modernizing the presentation of annotations" to "modernizing the presentation of annotations using tables" in the first paragraph?

eshmoylova avatar Apr 04 '22 12:04 eshmoylova

The title of the PR looks good. But I wonder if the description should be updated a bit as well. Maybe just adding updating "modernizing the presentation of annotations" to "modernizing the presentation of annotations using tables" in the first paragraph?

Sure, no problem. Done.

henrikt-ma avatar Apr 04 '22 20:04 henrikt-ma