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

Problem with final qualifier in MultiPhase models #2318

Closed
raulrpearson opened this issue Sep 6, 2017 · 19 comments
Closed

Problem with final qualifier in MultiPhase models #2318

raulrpearson opened this issue Sep 6, 2017 · 19 comments
Assignees
Labels
bug Critical/severe issue L: Electrical.Analog Issue addresses Modelica.Electrical.Analog L: Electrical.MultiPhase Issue addresses Modelica.Electrical.MultiPhase L: Electrical.QuasiStationary Issue addresses Modelica.Electrical.QuasiStationary
Milestone

Comments

@raulrpearson
Copy link

I tried to simulate a very simple multiphase circuit (a sine voltage source in parallel with a resistive load) and got this problem:

Overriding final modifier for T
File: .../Modelica/Modelica/Electrical/Analog/Basic.mo, line 44
Overriding found at:
File: .../Modelica/Modelica/Electrical/MultiPhase.mo, line 1105

I believe this is due to @beutlich's latest 970c5b5 commit. Similar to what is discussed by @christiankral in #2307.

The problem in this case lies, in my opinion, in Multiphase.Basic.Resistor:

// ...
extends Modelica.Electrical.MultiPhase.Interfaces.ConditionalHeatPort(final mh=m,
   final T=T_ref);
Modelica.Electrical.Analog.Basic.Resistor resistor[m](
   final R=R,
   final T_ref=T_ref,
   final alpha=alpha,
   each final useHeatPort=useHeatPort,
   final T=T) annotation (Placement(transformation(extent={{-10,-10},{10,10}})));
    equation
connect(resistor.p, plug_p.pin)
// ...

I have the impression that this applies to all of the models in MultiPhase that include conditional heat ports.

Additionally, shouldn't all of the modifications to the parameters of resistor use the each keyword? It's certainly not required, because I'm able to run the simulation after deleting final T=T, but wouldn't it be more correct to include that?

@beutlich
Copy link
Member

beutlich commented Sep 7, 2017

@raulrpearson

  1. Could you please add your test model. Either inline in your answer (if a few lines only) or as gist. Thanks.
  2. Does the problem also occur if you revert 970c5b5? If no, I am not sure if it is a Multiphase inheritance issue or an erroneous commit.
  3. About the each question: R , T_ref and alpha are already of dimension m, so each must not be set for these modifiers.

@beutlich beutlich added bug Critical/severe issue L: Electrical.MultiPhase Issue addresses Modelica.Electrical.MultiPhase labels Sep 7, 2017
@raulrpearson
Copy link
Author

This is the test model:

within ;
model MultiPhaseFinalT
  "A simple example to showcase problem with modifier of T parameter"
  Modelica.Electrical.Analog.Basic.Ground ground
    annotation (Placement(transformation(extent={{-10,-60},{10,-40}})));
  Modelica.Electrical.MultiPhase.Basic.Star star annotation (Placement(
        transformation(
        extent={{-10,-10},{10,10}},
        rotation=270,
        origin={0,-20})));
  Modelica.Electrical.MultiPhase.Basic.Resistor resistor(R={2,2,2}) annotation
    (Placement(transformation(
        extent={{-10,-10},{10,10}},
        rotation=270,
        origin={20,10})));
  Modelica.Electrical.MultiPhase.Sources.SineVoltage sineVoltage(V={10,10,10},
      freqHz={50,50,50}) annotation (Placement(transformation(
        extent={{-10,-10},{10,10}},
        rotation=270,
        origin={-20,10})));
equation
  connect(sineVoltage.plug_p, resistor.plug_p)
    annotation (Line(points={{-20,20},{0,20},{20,20}}, color={0,0,255}));
  connect(sineVoltage.plug_n, resistor.plug_n)
    annotation (Line(points={{-20,0},{0,0},{20,0}}, color={0,0,255}));
  connect(ground.p, star.pin_n)
    annotation (Line(points={{0,-40},{0,-30}}, color={0,0,255}));
  connect(star.plug_p, resistor.plug_n)
    annotation (Line(points={{0,-10},{0,0},{20,0}}, color={0,0,255}));
  annotation (uses(Modelica(version="3.x.x")), experiment(StopTime=0.1));
end MultiPhaseFinalT;

Reverting 970c5b5 does solve the problem.

@beutlich
Copy link
Member

beutlich commented Sep 7, 2017

Resolved in master by 43c6cd8.

@beutlich beutlich closed this as completed Sep 7, 2017
@beutlich beutlich added this to the MSL_next-MINOR-version milestone Sep 7, 2017
@christiankral
Copy link
Contributor

christiankral commented Sep 19, 2017

After updating the documentation according to #2276 and committing to Github I realized that this issue is not yet resolved. There are still errors caused by the final temperature parameters in the following libraries:

  • Modelica.Electrical.Machines
  • Modelica.Electrical.PowerConverters
  • Modelica.Magnetic.FundamentalWave
  • Modelica.Magnetic.QuasiStatic.FundamentalWave

@beutlich please check the examples of the listed Modelica sub packages.

@christiankral christiankral reopened this Sep 19, 2017
@beutlich
Copy link
Member

beutlich commented Sep 19, 2017

@christiankral Should Modelica.Magnetic.FundamentalWave.BasicMachines.Components.SinglePhaseWinding.resistor use TRef or TOperational as reference temperature? This looks like a real modeling bug to me that was not covered by MSL 3.2.2 (without the changes of #2307).

Edit: Same issue in Modelica.Magnetic.QuasiStatic.FundamentalWave.BasicMachines.Components.QuasiStaticAnalogWinding.resistor.

@beutlich
Copy link
Member

beutlich commented Sep 19, 2017

@christiankral Can you please point me to the issue in Modelica.Electrical.PowerConverters as I did not found any invalid occurence of overwriting a final modification!

@christiankral
Copy link
Contributor

In PowerConverters, this issue refers to examples which use a DC machine model:

  • Modelica.Electrical.PowerConverters.Examples.ACDC.RectifierBridge2Pulse.ThyristorBridge2Pulse_DC_Drive
  • Modelica.Electrical.PowerConverters.Examples.ACDC.RectifierBridge2mPulse.ThyristorBridge2mPulse_DC_Drive
  • Modelica.Electrical.PowerConverters.Examples.DCDC.HBridge.HBridge_DC_Drive

@beutlich
Copy link
Member

@christiankral I resolved the obvious ones by 6e618d5, but leave the tricky ones (Modelica.Magnetic.FundamentalWave.BasicMachines.Components.SinglePhaseWinding.resistor and Modelica.Magnetic.QuasiStatic.FundamentalWave.BasicMachines.Components.QuasiStaticAnalogWinding.resistor) to you.

@beutlich
Copy link
Member

@christiankral Modelica.Magnetic.FundamentalWave.BasicMachines.Components.SymmetricMultiPhaseWinding has unsued parameter TOperational. To me it looks like TOperational can be removed from all these models and only have TRef. This will solve this issue.

@christiankral
Copy link
Contributor

OK, I will have a look at it in the next days.

@christiankral
Copy link
Contributor

When extending from the conditional heat port, the parameter T must not be set final. The reason is, that if the heat port is not present, the operational temperature T is used. The default value of the operational temperature T is equal to T_ref, but the attribute final is wrong, as the user may pick a operating temperature differing from T_ref.

I consequently removed the attribute final in 90425cc and now the machine models check OK again.

@christiankral christiankral added L: Electrical.Analog Issue addresses Modelica.Electrical.Analog L: Electrical.QuasiStationary Issue addresses Modelica.Electrical.QuasiStationary labels Sep 24, 2017
@christiankral
Copy link
Contributor

I added tags according to affected packages

@beutlich
Copy link
Member

Hmm, your commit 90425cc is unexpected to me and to be honest, I do not get it.

  • It basically reverts Final modifier inheritance in Electrical.Analog sources #2307, that is parameters used as modifiers in extends clause should be declared final.
  • Parameter TOperational still is unsued in Magnetic.FundamentalWave.BasicMachines.Components.SymmetricMultiPhaseWinding, Magnetic.FundamentalWave.BasicMachines.Components.SymmetricMultiPhaseCageWinding_obsolete, Magnetic.QuasiStatic.FundamentalWave.BasicMachines.Components.SymmetricMultiPhaseCageWinding.

I created a tiny example package for easier testing. Revision 1 shows the current implementation in MSL master (w/o final temperature). Revision 2 are my proposed changes to set the temperature modifier T final.

@HansOlsson Can you please advice and give me a clue if final is to be used or not, that is Revision 2 or 1? Thanks a lot!

@beutlich beutlich reopened this Sep 25, 2017
@HansOlsson
Copy link
Contributor

HansOlsson commented Sep 25, 2017

To me it makes sense to separate reference and operational temperature (so "Revision 1" seem correct).

The reference is a reference - so that e.g. in a resistor we specific R at reference temperature, and its linear temperature dependency.

The operational temperature is the temperature it is actually used at; unless we have a heat-port and let this vary dynamically (to add to the confusion the ConditionalHeatport this is called "T" and not "Toperational").

On one hand it makes sense that they as default are the same, but on the other hand it also makes sense to specify them separately (if you have a circuit with specified resistor and want to see what happens if you heat it up), and thus the correct solution seems to be to not have "final" modifier for T.

But it also seems clear that some more documentation is needed for this - I hope my description above is right, but I don't see it explicitly described anywhere. A user should be able to understand why to set T and T_ref differently without having to understand the models in this detail.

@christiankral
Copy link
Contributor

Attributes final introduced in 970c5b5 to conditional heat ports are removed for:

  • Modelica.Electrical.Machines 89f9071
  • Modelica.Electrical.Analog.Semiconductors 8839ea8

Changes, which still may have to be considered, are (I need more time for this):

  • Modelica.Magnetic.FluxTubes.Interfaces.ConditionalHeatPort
  • Modelica.Electrical.Analog.Interfaces.ConditionalHeatPort
  • Modelica.Thermal.HeatTransfer.Interfaces
    • PartialConditionalHeatPort
    • PartialElementaryConditionalHeatPort
    • PartialElementaryConditionalHeatPortWithoutT

@christiankral
Copy link
Contributor

See also #2312

@beutlich
Copy link
Member

Thanks @HansOlsson for clarification.

I can now see that the trouble comes from the Resistor.T_ref parameter which is used both as reference temperature (for the linear temperature dependency of the resistance) and as operational temperature (if no conditional heat port is available). I updated the demo example package by Revision 3 which shows the clean solution I was thinking of. Setting the new parameter TOperational = T_ref by default in the Resistor model whould guarantee backward compatibility.

The current implementation (i.e., Revision 1) is a short hand notation in order to not introduce another temperature TOperational at the Resistor model, which then requires to have no final modification for T. This is kind of OK but still confusing and I agree, that it should be better noted in the documentation. Even more confusing is that TOperational then is introduced in the component models (e.g., M in my demo example) just to pass it to the resistor instance.

beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this issue Sep 27, 2017
T_ref is used both as reference temperature (for the linear temperature dependency of the resistance/conductance) and as operational temperature (if no conditional heat port is available). With the introduction of T_operational the confusion can be avoided by a clean separation of the two temperatures.

Furthermore, parameter T can now be modified as final.
@beutlich
Copy link
Member

@christiankral Anything left to do?

@christiankral
Copy link
Contributor

As #2312 is a separate ticket, I think we are fine on this issue and resolved everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Critical/severe issue L: Electrical.Analog Issue addresses Modelica.Electrical.Analog L: Electrical.MultiPhase Issue addresses Modelica.Electrical.MultiPhase L: Electrical.QuasiStationary Issue addresses Modelica.Electrical.QuasiStationary
Projects
None yet
Development

No branches or pull requests

4 participants