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

Non optional connector #3129

Merged
merged 17 commits into from
Jul 1, 2022
Merged

Conversation

HansOlsson
Copy link
Collaborator

Closes #3117 (see that ticket for more information).
This needs to be discussed in the group, but the main idea is to revert #178 and instead have annotations for indicating restrictions on connections.

For MSL (and other libraries) the change has two main benefits:

@HansOlsson
Copy link
Collaborator Author

HansOlsson commented Mar 3, 2022

@casella Do you agree that this makes sense for stream-connectors?

In particular this would generate diagnostics for the upper two variants in Modelica.Fluid.Examples.Explanatory.MeasuringTemperature (but not for the bottom one).
This assumes that the cardinality-assertion in PartialLumpedVessel is replaced by an annotation.
Note that the example currently circumvents the assertions while still generating the same connection-set; I don't think that is good and one of the benefits of the annotation is that it is avoided.

@HansOlsson HansOlsson requested a review from casella March 3, 2022 15:34
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
Add spaces

Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
@HansOlsson HansOlsson added this to the Phone 2022-2 milestone Apr 21, 2022
@HansOlsson
Copy link
Collaborator Author

Agreement - but change to error.

@HansOlsson HansOlsson requested a review from henrikt-ma May 4, 2022 12:23
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
chapters/annotations.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting some changes, but would also like this raise the question of where to present these annotations. In what way are they related to Graphical User Interface? Wouldn't Symbolic Processing make more sense, or to combine the new annotations with Single Use of Class (renaming the section title to something like Usage Restrictions)?

Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
@HansOlsson
Copy link
Collaborator Author

Requesting some changes, but would also like this raise the question of where to present these annotations. In what way are they related to Graphical User Interface? Wouldn't Symbolic Processing make more sense, or to combine the new annotations with Single Use of Class (renaming the section title to something like Usage Restrictions)?

The latter seems the best.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only important thing I see missing is an explanation of what "flow variable may be negative" means here.

@HansOlsson
Copy link
Collaborator Author

The only important thing I see missing is an explanation of what "flow variable may be negative" means here.

This has now been added.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from minor details regarding the example, the only real question concerns how to – if at all – deal with the fact that the min attribute isn't ensured to be constant.

@HansOlsson
Copy link
Collaborator Author

Apart from minor details regarding the example, the only real question concerns how to – if at all – deal with the fact that the min attribute isn't ensured to be constant.

The idea is to deal with that part of streams-connectors in exactly the same way as other simplifications for them:
https://specification.modelica.org/master/derivation-of-stream-equations.html#connection-of-3-stream-connectors-where-one-mass-flow-rate-is-identical-to-zero-n-3-and

HansOlsson and others added 3 commits June 13, 2022 09:12
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this looks good now. However, think it would be reassuring to also get a review from someone more on the application side of things, Modelica.Mechanics as well as Modelica.Fluid

@casella
Copy link
Collaborator

casella commented Jun 13, 2022

@HansOlsson I'm sorry but I missed the original request for review, and then many things happened in the meantime. What should I review at this point, exactly?

@HansOlsson
Copy link
Collaborator Author

@HansOlsson I'm sorry but I missed the original request for review, and then many things happened in the meantime. What should I review at this point, exactly?

If the rules for the annotation to restrict maximum number of connections makes sense, as potential replacement for the cardinality-assertions in Modelica.Fluid. (The minimum number of connectors is a separate issue - I might split it into two if this discussion gets too complicated.)

Thinking more I'm a bit unsure. On one hand the new rules make sense, and on the other hand it would mean that an example in MSL is misleading. It's a sort of trade-off: how much effort do we want to spend on avoiding potential pit-falls?

The basic idea with the restriction is to replace cardinality-checks.

And since Modelica mostly deal with connection-sets instead of connections the idea was instead of restricting the number of connections to the connector it restricts the number of connectors in its connection set (excluding sensors for fluid, since otherwise it would break more). That implies that the top-two variants in Modelica.Fluid.Examples.Explanatory.MeasuringTemperature would trigger it (if using the annotation instead of the current cardinality-assert). The bottom one would not - since the junction makes the intention clear.
The sensor-part means that Modelica.Fluid.Examples.HeatingSystem will not trigger this new annotation.

The MeasuringTemperature example is a bit special: since there is no other dynamics we never get flow directly from one tank to the next - but only from/to the mass-flow.

I can see the following possibilities for the check in OpenTank (well, its base-class):

  • Remove it completely. Users should know what they do.
  • Use new annotation as proposed - only one other connector, excluding sensors. Will require junctions in these cases (if we use the new annotation).
  • Check that only connected to one other connector (similar to current cardinality; that would require updating this PR and make it simpler). That means that you can circumvent the cardinality-check by instead of connecting two connectors to a tank-port you add a temperature sensor and connect all of them (and the tank-port) to the sensor.

On one hand using connector-sets seem more correct but:

  • The rules are more complicated.
  • You still cannot directly connect a sensor graphically to a tank-port due to connectorSizing not considering that case; so in practice it doesn't gain much.

@HansOlsson HansOlsson merged commit 2e520c5 into modelica:master Jul 1, 2022
@HansOlsson HansOlsson removed the request for review from casella July 1, 2022 11:42
@HansOlsson HansOlsson added the M36 For pull requests merged into Modelica 3.6 label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M36 For pull requests merged into Modelica 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent optional diagnostics for unconnected connectors.
3 participants