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

MCP-0033 Predefined Plots: comments on specification #2578

Open
DagBruck opened this issue Jun 12, 2020 · 4 comments
Open

MCP-0033 Predefined Plots: comments on specification #2578

DagBruck opened this issue Jun 12, 2020 · 4 comments
Labels
discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP) MCP0033 Predefined plot issues and PRs (MCP-0033)
Milestone

Comments

@DagBruck
Copy link
Collaborator

Having implemented (most of) MCP-0033 in Dymola, we have a few comments on the specification and suggestions for minor changes. In any event, our conclusion is that the MCP is ready for inclusion in the specification.

  • Curves: We noted that the curve properties do not include things like line color, line style or markers. That simplifies the proposal, but would be possible extensions.
  • Curves: I suggest adding this text for completeness sake:

    If x or y does not designate a variable the behavior is unspecified.

  • Axis: I suggest the following addition:

    If the specified unit is not supported by the tool, or the unit does not represent a display unit of the plotted variable (for example, unit="cm" and variable denotes a mass), the behavior is unspecified.

  • Axis: "label shall not convey unit information" can be re-phrased as "label shall not contain the unit".
  • Variable replacements and links are not implemented in Dymola, and it is not obvious that a tool needs to support them. For that reason I suggest we make them optional. Adding

    Support for variable replacements is optional.
    ...
    Support for links is optional.

@DagBruck DagBruck added MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP) discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended MCP0033 Predefined plot issues and PRs (MCP-0033) labels Jun 12, 2020
otronarp added a commit to otronarp/ModelicaSpecification that referenced this issue Jun 12, 2020
Clarify that behavior is unspecified for:
 - Curves where x or y does not designate a variable
 - Unknown units or incompatible with the plotted variable
@otronarp
Copy link
Member

  • Curves: We noted that the curve properties do not include things like line color, line style or markers. That simplifies the proposal, but would be possible extensions.

That was a decision made at some design metting to simplify the proposal inorder to be able to specify something useful quicker. As you say we can always revisit it in the future and extend it with support for styling.

  • Curves: I suggest adding this text for completeness sake:

    If x or y does not designate a variable the behavior is unspecified.

  • Axis: I suggest the following addition:

    If the specified unit is not supported by the tool, or the unit does not represent a display unit of the plotted variable (for example, unit="cm" and variable denotes a mass), the behavior is unspecified.

  • Axis: "label shall not convey unit information" can be re-phrased as "label shall not contain the unit".

I created a PR for that: #2579.

  • Variable replacements and links are not implemented in Dymola, and it is not obvious that a tool needs to support them. For that reason I suggest we make them optional. Adding

    Support for variable replacements is optional.
    ...
    Support for links is optional.

Am not aware of that we talk about optional features in the specification, I thought the usual wording for features that are kind of optional is to say that it's tool specific. In that sense links are optional:

The styling of the link text and the link action is left for each Modelica tool to decide.

For variable replacement I'm reluctant to call them optional, but it might be a good idea to define what should happen if a tool cannot replace the variable (for whatever reason).

@DagBruck
Copy link
Collaborator Author

For connectorSizing, the current specification says:

If connectorSizing = true, a tool may set the parameter value in a modifier automatically, if used as dimension size of a vector of connectors.

Hence my suggestion would be:

If links or variable replacements are used, a tool may use them to substitute the references in the text.

@henrikt-ma
Copy link
Collaborator

I would avoid writing it that way, as it makes it sound as if a valid implementation doesn't need to do anything to the substring %{inertia1.w}. Clearly, this will look bad in the resulting figure, and must be seen as an error in the implementation; it's not the library developer's fault that the figure looks ugly. When a simulation result for inertia1.w isn't available, I think it is best to leave it up to the tools to decide how this is best communicated to the user; these are just some better or worse possibilities from the top of my head, none of which I'd like to see enforced by the specification:

  • Some tools may be aware that this variable exists, but that its result wasn't stored, and might choose to display this as <Not stored> with a context menu providing alternatives for turning on storage for this variable.
  • Some tools may be able to tell that inertia1.w isn't a valid variable name at all, and might choose to show <Invalid reference> in red, with a tooltip showing the name of the missing variable.
  • Some tools may not care to do a deeper analysis and just show %{inertia1.w} in (blinking, of course) red.

henrikt-ma added a commit that referenced this issue Aug 10, 2020
@henrikt-ma henrikt-ma added this to the Phone2020-4 milestone Aug 11, 2020
@henrikt-ma
Copy link
Collaborator

I added this to the https://github.com/modelica/ModelicaSpecification/milestone/74 milestone so that the part remaining after merging #2579 can be discussed, namely how to formulate the specification of variable replacements.

As indicated above, I think that the current formulation is the right one, but a compromise to meet the request in this issue might be to add some non-normative text to clarify that implementing variable replacements doesn't have to be hard…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Indicates that there's a discussion; not clear if bug, enhancement, or working as intended MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP) MCP0033 Predefined plot issues and PRs (MCP-0033)
Projects
None yet
Development

No branches or pull requests

3 participants