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

Set Evaluate = true for IdealTransformer.considerMagnetization #3987

Conversation

henrikt-ma
Copy link
Contributor

Leaving this parameter unevaluated seems like making things overly complicated, and as far as I can see there is nothing that clearly requires the parameter to be treated structurally. Hence, adding an explicit annotation will increase chances of models being treated equally efficiently across tools.

Of course, I would have preferred to declare considerMagnetization a constant with Dialog annotation instead, but I'm afraid the general preference is to use a parameter with Evaluate = true instead.

@henrikt-ma henrikt-ma added the L: Electrical.Polyphase Issue addresses Modelica.Electrical.Polyhase label May 23, 2022
@henrikt-ma
Copy link
Contributor Author

There's a similar situation in Modelica.Thermal.FluidHeatFlow.BaseClasses.TwoPort that would be interesting to discuss at the same time, namely the parameter m present in this if-equation condition:

  if m>Modelica.Constants.small then
    flowPort_a.H_flow + flowPort_b.H_flow + Q_flow = m*medium.cv*der(T);
  else
    flowPort_a.H_flow + flowPort_b.H_flow + Q_flow = 0;
  end if;

Would you say it's a Modelica tool "frontend" problem to realize that the condition of this tricky equation should be evaluated, or would you expect that index reduction is the stage when one should determine which parametric if-equation conditions to evaluate?

Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

Fine with wem.
Although I don't get the connection to Modelica.Thermal.FluidHeatFlow.BaseClasses.TwoPort.

@henrikt-ma
Copy link
Contributor Author

Nobody has any thoughts regarding the question about Modelica.Thermal.FluidHeatFlow.BaseClasses.TwoPort above?

Copy link
Contributor

@christiankral christiankral left a comment

Choose a reason for hiding this comment

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

OK with me.

@beutlich beutlich enabled auto-merge (squash) August 10, 2023 07:36
@beutlich beutlich force-pushed the evaluate-considermagnetization branch from f78dc41 to b2720c1 Compare August 10, 2023 07:37
@beutlich beutlich added this to the MSL4.1.0 milestone Aug 10, 2023
@beutlich beutlich added L: Electrical.Analog Issue addresses Modelica.Electrical.Analog and removed L: Electrical.Polyphase Issue addresses Modelica.Electrical.Polyhase labels Aug 10, 2023
@beutlich beutlich disabled auto-merge August 10, 2023 07:39
Leaving this parameter unevaluated seems like making things overly complicated, and as far as I can see there is nothing that clearly requires the parameter to be treated structurally.  Hence, adding an explicit annotation will increase chances of models being treated equally efficiently across tools.

Of course, I would have preferred to declare considerMagnetization a constant with Dialog annotation instead, but I'm afraid the general preference is to use a parameter with Evaluate = true instead.
@beutlich beutlich force-pushed the evaluate-considermagnetization branch from b2720c1 to 54c07f2 Compare January 15, 2024 05:52
@arunkumar-narasimhan arunkumar-narasimhan merged commit 7e2ba44 into modelica:master Jan 16, 2024
2 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants