Prepare for CI: Magnetic.QuasiStatic.FundamentalWave#4787
Prepare for CI: Magnetic.QuasiStatic.FundamentalWave#4787AHaumer wants to merge 4 commits intomodelica:masterfrom
Conversation
…tic.FundamentalWave
|
@AHaumer Some "new" models have been added to ModelicaTest. Why can't we just extend these models from the MSL and add a proper annotation? They look quite identical to me. |
|
Extending from the Modelica Standard Library make the code very maintainable. One more aspect here:
extends Modelica.Magnetic.QuasiStatic.FundamentalWave.Examples.BasicMachines.SynchronousMachines.SMPM_CurrentSource;
annotation (
experiment(StopTime=0.20, Interval=1E-4, Tolerance=1E-6),
TestCase(shouldPass = true,
__ModelicaAssociation(Comparison(TimeWindows={TimeSlot(0.00, 0.20)}))),
Documentation(info="<html>
<p>
This example compares a time transient and a quasi-static model of a permanent magnet synchronous machine. The machines are fed by a current source. The current components are oriented at the magnetic field orientation and transformed to the stator fixed reference frame. This way the machines are operated at constant torque. The machines start to accelerate from standstill.</p>
<p>
Simulate for 2 seconds and plot (versus time):
</p>The stop time is set to 0.20 seconds and the documentation states "Simulate for 2 seconds". I wonder if it makes more sense to just state something like: Reasons:
|
I don't see this being OK regarding the To make time windowed comparison beyond just taking the initial part of the result of a chaotic system, we need this |
|
@christiankral I adreesed your concerns. Sorry for not using extends, my fault. |
I was commenting on the following (emphasis is mine):
That is, if you remove the start and, and maybe stop calling it a time window when the window must be open-eded to the left, then it would seem OK to me as well. I would rather call what you are doing something like a short-time comparison model to not cause confusion with a proper time-windowed comparison which we seem to need sooner or later anyway. |
I agree that we shouldn't duplicate the documentation, but to me it seems simplest to as default not have any documentation for these models. Tools can already show that it extends from another model, and in some way make that accessible. In Dymola that would currently give something like:
(But with an internal link instead.) Thus, I don't see that spending time on writing "The original documentation is available at the model from which this one is extended." really helps. I could even imagine that tools could be more helpful if the documentation of the derived class is empty, so it might be that the text is counter-productive. |
| Documentation( | ||
| info="<html> | ||
| <p> | ||
| The original documentation is available at the model from which this one is extended. | ||
| </p> | ||
| </html>"), |
There was a problem hiding this comment.
| Documentation( | |
| info="<html> | |
| <p> | |
| The original documentation is available at the model from which this one is extended. | |
| </p> | |
| </html>"), |
No need for documentation to say that it extends from something.
| Documentation( | ||
| info="<html> | ||
| <p> | ||
| The original documentation is available at the model from which this one is extended. | ||
| </p> | ||
| </html>"), |
There was a problem hiding this comment.
| Documentation( | |
| info="<html> | |
| <p> | |
| The original documentation is available at the model from which this one is extended. | |
| </p> | |
| </html>"), |
No need for documentation to say that it extends from something.
| Documentation( | ||
| info="<html> | ||
| <p> | ||
| The original documentation is available at the model from which this one is extended. | ||
| </p> | ||
| </html>"), |
There was a problem hiding this comment.
| Documentation( | |
| info="<html> | |
| <p> | |
| The original documentation is available at the model from which this one is extended. | |
| </p> | |
| </html>"), |
No need for documentation to say that it extends from something.
| Documentation( | ||
| info="<html> | ||
| <p> | ||
| The original documentation is available at the model from which this one is extended. | ||
| </p> | ||
| </html>"), |
There was a problem hiding this comment.
| Documentation( | |
| info="<html> | |
| <p> | |
| The original documentation is available at the model from which this one is extended. | |
| </p> | |
| </html>"), |
No need for documentation to say that it extends from something.
| Documentation( | ||
| info="<html> | ||
| <p> | ||
| The original documentation is available at the model from which this one is extended. | ||
| </p> | ||
| </html>"), |
There was a problem hiding this comment.
| Documentation( | |
| info="<html> | |
| <p> | |
| The original documentation is available at the model from which this one is extended. | |
| </p> | |
| </html>"), |
No need for documentation to say that it extends from something.
| Electrical | ||
| Fluid | ||
| Magnetic | ||
| Media |
There was a problem hiding this comment.
| Electrical | |
| Fluid | |
| Magnetic | |
| Media | |
| Electrical | |
| Magnetic | |
| Fluid | |
| Media |
I understand that it looked odd to have "Magnetic" last, but I assume it is an expanded view of the Modelica-structure not a pure alphabetic order (that's why Media is before Math and Tables after Blocks), so I think Magnetic should be grouped with Electrical, not between Fluid and Media.
It could also be that it is just based on when the packages were added, but even in that case I think we should start working towards a logical order based on MSL - not an alphabetical one.
HansOlsson
left a comment
There was a problem hiding this comment.
Some minor issues, but overall it looks good.
@HansOlsson @AHaumer I totally agree. The simplest and most transparent solution is just to not add any documentation and leave it up to the tool. |
HansOlsson
left a comment
There was a problem hiding this comment.
LGTM - except for the package.order comment.
same procedure as #4775, except Modelica.Magnetic.QuasiStatic.FundamentalWave.Examples.BasicMachines.InductionMachines.IMC_withLosses where IMHO a longer interval (factor 100) is sufficient to reduce the size of the reference results.