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

MassFlowSource_T and MassFlowSource_h only work if there are at least one port #4461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HansOlsson
Copy link
Contributor

The two models Modelica.Fluid.Sources.MassFlowSource_T and Modelica.Fluid.Sources.MassFlowSource_h only work if there is at least one port - since they set a mass-flow and you cannot have a mass-flow without ports.

Due to the design of connectorSizing we should clearly not change the default value, but I instead propose to add a min-value.

The reason for this change is two-fold:

  • Avoid false positives when checking the library; as the default configuration shouldn't work.
  • When (mis)using these components the error can be detected early.

@HansOlsson HansOlsson added the L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation) label Sep 5, 2024
@beutlich beutlich changed the title These two sources only work if there are at least one port. MassFlowSource_T and MassFlowSource_h only work if there are at least one port Sep 6, 2024
@casella casella self-requested a review September 17, 2024 09:35
Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

LGTM in principle, but we already have

assert(nPorts > 0, "At least one port needs to be present (nPorts > 0), otherwise the model is singular");

Isn't that redundant?

@HansOlsson
Copy link
Contributor Author

HansOlsson commented Sep 17, 2024

LGTM in principle, but we already have

assert(nPorts > 0, "At least one port needs to be present (nPorts > 0), otherwise the model is singular");

Isn't that redundant?

Well, I would say there's overlap - not that they are redundant.

  • The disadvantage with asserts is that it depends on when they are checked - clearly such a model cannot simulate, but exactly when it fails isn't clear. In contrast a min-value for an evaluable parameter can clearly be checked when we have a value.
  • The disadvantage with a min-value in general is that it isn't clear what the problem is. However, for this specific case the combination of connectorSizing and min-value clearly indicates the issue tools can easily detect that; whereas the assert as currently written isn't very clear - it discusses nPorts, but a user should ideally not see that parameter.

@casella casella self-requested a review September 17, 2024 10:18
Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Fluid Issue addresses Modelica.Fluid (excl. Dissipation)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants