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

Fix annotations #4147

Merged
merged 7 commits into from
Sep 12, 2023
Merged

Fix annotations #4147

merged 7 commits into from
Sep 12, 2023

Conversation

HansOlsson
Copy link
Contributor

Avoid using unknown parameters in annotations.

Only one of the changes is somewhat controversial - but having enable evaluating to false on a function input without a default-value simply doesn't work, and I'm not comfortable with adding a default value.

@beutlich beutlich added L: Electrical.Analog Issue addresses Modelica.Electrical.Analog L: Fluid.Dissipation Issue addresses Modelica.Fluid.Dissipation L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody labels Jul 2, 2023
@tobolar
Copy link
Contributor

tobolar commented Aug 8, 2023

Only one of the changes is somewhat controversial ...

@HansOlsson You mean input Real epsilon_A in Fluid.Dissipation.Utilities.Functions.PressureLoss.TwoPhase.TwoPhaseDensity, right? I think the input description could be optionally enhanced by something like "relevant for heterogeneous approaches".

But looking in the calculation of rho_mom and rho_kin, the input epsilon_A seems have to be given anyway, so enable annotation makes no sense.

Regarding a default value, IMO this function would be used by an experienced user who knows about necessary inputs for homogenous/heterogenous approaches. So maybe a default value could be acceptable here.

@HansOlsson
Copy link
Contributor Author

Only one of the changes is somewhat controversial ...

@HansOlsson You mean input Real epsilon_A in Fluid.Dissipation.Utilities.Functions.PressureLoss.TwoPhase.TwoPhaseDensity, right? I think the input description could be optionally enhanced by something like "relevant for heterogeneous approaches".

Yes.
I agree adding that description and possible default value would be possible, but I view that as a further enhancements that can be done later in a separate PR.

Copy link
Contributor

@tobolar tobolar left a comment

Choose a reason for hiding this comment

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

Fine to me.

@HansOlsson HansOlsson merged commit 438d8f8 into modelica:master Sep 12, 2023
2 checks passed
@HansOlsson HansOlsson deleted the FixAnnotations branch September 12, 2023 13:03
@beutlich beutlich added this to the MSL4.1.0 milestone Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Electrical.Analog Issue addresses Modelica.Electrical.Analog L: Fluid.Dissipation Issue addresses Modelica.Fluid.Dissipation L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants