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

MSL 4.1.0 Regressions - Electrical and ComplexBlocks #4333

Open
10 of 11 tasks
GallLeo opened this issue Feb 27, 2024 · 23 comments
Open
10 of 11 tasks

MSL 4.1.0 Regressions - Electrical and ComplexBlocks #4333

GallLeo opened this issue Feb 27, 2024 · 23 comments
Assignees
Labels
example Issue only addresses example(s) L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath L: Electrical.Machines Issue addresses Modelica.Electrical.Machines L: Electrical.PowerConverters Issue addresses Modelica.Electrical.PowerConverters L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3 L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Milestone

Comments

@GallLeo
Copy link
Contributor

GallLeo commented Feb 27, 2024

The following models fail in result comparison.
Tested revision: f9bddf8 (2024-02-16)

Changed models, need reference update after library officer check:

  • Modelica.Electrical.Machines.Examples.ControlledDCDrives.CurrentControlledDCPM
    - Reason: Erroneous initialization of armatureInverter.iDCFilter has been fixed.
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.Electrical.Machines.Examples.ControlledDCDrives.PositionControlledDCPM
    - Reason: Erroneous initialization of armatureInverter.iDCFilter has been fixed.
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.Electrical.Machines.Examples.ControlledDCDrives.SpeedControlledDCPM
    - Reason: Erroneous initialization of armatureInverter.iDCFilter has been fixed.
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.Electrical.PowerConverters.Examples.DCAC.PolyphaseTwoLevel.ThreePhaseTwoLevel_PWM
    - Reason: Bugfix in Modelica.Electrical.PowerConverters.DCAC.Control.SVPWM
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.Electrical.PowerConverters.Examples.DCAC.SinglePhaseTwoLevel.SinglePhaseTwoLevel_R
    - Note: does not fail in csv-compare, but in integral check for inverter.ac.v
    - Reason: Changed formulation of comparison in Modelica.Electrical.PowerConverters.DCDC.Control.SignalPWM
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.Electrical.PowerConverters.Examples.DCDC.HBridge.HBridge_R
    - Reason: Changed formulation of comparison in Modelica.Electrical.PowerConverters.DCDC.Control.SignalPWM
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.Electrical.Spice3.Examples.Spice3BenchmarkFourBitBinaryAdder
    - Reason: Model has not been touched for ages, differences are neglectible.
    @GallLeo pls. create new reference results.
    - Updated reference files?
  • Modelica.ComplexBlocks.Examples.ShowTransferFunction
    - Reason: Changed the parameters of the example to get more eye-catching results.
    @GallLeo pls. create new reference results.
    - Updated reference files?

Not classified yet, needs library officer check:

  • Modelica.Electrical.Machines.Examples.InductionMachines.IMC_InverterDrive
    - Reason: Bugfix in Modelica.Electrical.PowerConverters.DCAC.Control.SVPWM
    @GallLeo pls. create new reference results.
    - Updated reference files?

  • @GallLeo please note: We have improved the comparisonSignals.txt in PR comparisonSignals of PowerConverters #4353 of Modelica.Electrical.PowerConverters.Examples.DCDC.HBridge.{HBridge_R, HBridge_RL, HBridge_DC_Drive}, also back-ported to maint/4.1.x in comparisonSignals of PowerConverters #4372.
    Please create new reference results for all three examples.

  • Release notes check: Are all classes mentioned which could lead to result changes in user models?


Useful Links

Current comparison report:
https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/comparison_report_overview.html
-> Reference result test -> Comparison

Comparison signal definitions:
https://github.com/modelica/ModelicaStandardLibrary/tree/master/Modelica/Resources/Reference/Modelica
https://github.com/modelica/ModelicaStandardLibrary/tree/master/ModelicaTest/Resources/Reference/ModelicaTest

Reference results:
https://github.com/modelica/MAP-LIB_ReferenceResults

@GallLeo GallLeo added L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath L: Electrical.Machines Issue addresses Modelica.Electrical.Machines L: Electrical.PowerConverters Issue addresses Modelica.Electrical.PowerConverters L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3 L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined example Issue only addresses example(s) V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases) labels Feb 27, 2024
@GallLeo GallLeo added this to the MSL4.1.0 milestone Feb 27, 2024
@maltelenz maltelenz changed the title MSL 4.1.0 Regressions - Electrical MSL 4.1.0 Regressions - Electrical and ComplexBlocks Feb 28, 2024
@maltelenz
Copy link
Contributor

@GallLeo please note: We have improved the comparisonSignals.txt in PR #4353 of Modelica.Electrical.PowerConverters.Examples.DCDC.HBridge.{HBridge_R, HBridge_RL, HBridge_DC_Drive}
Please create new reference results for all three examples.

@AHaumer The LTX run actually shows a correctness failure in Modelica.Electrical.PowerConverters.Examples.DCDC.HBridge.HBridge_DC_Drive for the variable meanCurrent.y, so that should be checked specifically before resetting the baseline to get the new variables.

@AHaumer
Copy link
Contributor

AHaumer commented Jun 6, 2024

@maltelenz @GallLeo
The only difference I see is at the beginning of the trajectory of meanCurrent,y.
This is caused from improvements in the block SignalPWM.
Yes the new signal is correct, and please take the new signal as reference for the future.

GallLeo added a commit to modelica/MAP-LIB_ReferenceResults that referenced this issue Oct 14, 2024
@GallLeo
Copy link
Contributor Author

GallLeo commented Oct 15, 2024

Regressions not mentioned in this ticket:

  1. Modelica.Electrical.Analog.Examples.OpAmps.SignalGenerator

  2. Modelica.Electrical.Machines.Examples.SynchronousMachines.SMPM_NoLoad

Should they also get new references @AHaumer ?

Current comparison report:
https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/Compare/Modelica/testrun_report.html

@maltelenz
Copy link
Contributor

2. Modelica.Electrical.Machines.Examples.SynchronousMachines.SMPM_NoLoad

This model was already reported in #4363

@AHaumer
Copy link
Contributor

AHaumer commented Oct 15, 2024

@GallLeo

  1. Modelica.Electrical.Analog.Examples.OpAmps.SignalGenerator
    opAmp1.out.v is a bad signal for comparison: only spikes.
    I suggest to remove opAmp1.out.v from comparisonSignals.txt and generate new reference results.

OUTDATED see below

@AHaumer
Copy link
Contributor

AHaumer commented Oct 15, 2024

@GallLeo
2. Modelica.Electrical.Machines.Examples.SynchronousMachines.SMPM_NoLoad
I admit details both of the model and the comparisonSignals.txt have been changed.
Please generate new reference results - thank you.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 15, 2024

@GallLeo Regarding the timeout of Modelica.Electrical.Machines.Examples.InductionMachines.IMC_YD:
I have no idea why the example takes so long to simulate with tolerance/10.
It seems that something goes wrong with the iteration when the machine is switched on at time = 0,1 s:
IMC_YD
@casella Could we keep the original tolerance for comparison?
@christiankral should we loosen the "normal" tolerance to 1e-5 so that the comparison tolerance gets 1e-6 and this works well?
I see no big difference in the results with tolerance = 1e-5 and tolerance = 1e-6.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 16, 2024

I tried to analyze the problem a little bit:
Problems with Modelica.Magnetic.QuasiStatic.FundamentalWave.Examples.BasicMachines.InductionMachines.IMC_YD is due to the fact that this example compares a QuasiStatic machine with a dynamic/transient one.
The example Modelica.Electrical.Machines.InductionMachines.IMC_YD shows no problems.
It seems that the problems stem from a combination of Modelica.Electrical.Polyphase.Ideal.IdealClosingSwitch and Modelica.Magnetic.FundamentalWave.BasicMachines.InductionMachines.IM_SquirrelCage.
If I remove the IdealCLosingSwitch from the example (i.e. the machine is switched on at time = 0), we encounter no problmes with tighter tolerance.
I'll analyze a little bit more with a different implementation of the switch to avoid the problems.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 16, 2024

Another quick solution:
Change the parameters of the idealCloser to Ron=1e-4 and Goff 1e-4 (both default 1e-5) and the example runs like a charm with tolerance 1e-6 and tolerance = 1e-7.
There's only negligible deviation in the results, mainly the current before the switch is closed grows from 0.5 mA by a factor of 10.
One more fact: With original parameters, the example simulates fine with OM and Dymola if you choose Algorithm = Cvode even with tolerance=1e-7.
@christiankral what's your opinion, what should we do?
@HansOlsson do you have an idea about the reason for this weird behavior (timeout with tolerance 1e-7)?

@christiankral
Copy link
Contributor

I am in favor of keeping the original simulation tolerances and switch paramerers. If switching the simulation algorithm to cvode solves the issue, this is my preferred the way to go.

If we would reduce simulation tolerances and switch parameters instead, the following question arises: Why not change these tolerances and paramerers throughout the entire MSL, if we can live with less accuracy...

@casella
Copy link
Contributor

casella commented Oct 18, 2024

I'm also definitely in favour of updating the parameters. We should not include in the MSL examples that are numerically challenged.

Regarding switching the simulation algorithm, unfortunately we don't have the standardized annotations to do so, only vendor-specific annotations. This should probably be discussed at some point, as there are some models that need certain kind of solvers to be simulated efficiently. For example, flexible multibody models have very fast and very poorly damped eigenvalues, which don't work well with the stability region of BDF codes like DASSL, which is horribly slow as it needs to take very small steps to avoid going unstable. Implicit Runge-Kutta algorithms such as RADAU instead work very well.

The question is, do we standardize individual solvers? Categories of solvers, such as implicit/explicit, single-step, multi-step? That's still unclear to me. Anyway, topic for another ticket 😃

@casella
Copy link
Contributor

casella commented Oct 20, 2024

Regarding the first issue with Modelica.Electrical.Analog.Examples.OpAmps.SignalGenerator, opAmp1.out.v should not have any spikes at all. This is the result obtained with OpenModelica:
immagine
which looks like a perfectly legitimate square wave between the two supply voltage levels. I'm not sure what is going on with the regression testing infrastructure, but there is for sure some issue there. Removing opAmp1.out.v doesn't seem like a good idea to me.

GallLeo added a commit to modelica/MAP-LIB_ReferenceResults that referenced this issue Oct 21, 2024
@GallLeo
Copy link
Contributor Author

GallLeo commented Oct 21, 2024

Regressions not mentioned in this ticket:

1. Modelica.Electrical.Analog.Examples.OpAmps.SignalGenerator

2. Modelica.Electrical.Machines.Examples.SynchronousMachines.SMPM_NoLoad

Should they also get new references @AHaumer ?

Current comparison report: https://www.ltx.de/download/MA/Compare_MSL_v4.1.0/Compare/Modelica/testrun_report.html

Status:

  1. Waiting for concensus between @casella and @AHaumer

  2. New reference is pushed.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 21, 2024

@casella you are right, I was mistaken. OM is correct, Dymola seems to have a problem with this example!
@GallLeo Let's keep the comparisonSignals.txt.
I have to investigate a little bit more in the evening, especially the influence of the parameter strict on the Advanced tab.

@HansOlsson
Copy link
Contributor

Regarding the first issue with Modelica.Electrical.Analog.Examples.OpAmps.SignalGenerator, opAmp1.out.v should not have any spikes at all.

How do you know that for the Modelica model?

After investigation: in Dymola it depends on how smooth is handled.

Importantly it is not an error in the automatic handling of smooth since:

  • Enabling events for smooth generates a solution without spikes (some warnings for convergence issues). Similar result as in OpenModelica.
  • Disabling events for smooth (i.e., treating it as noEvent) generates a solution with spikes, and some warnings for failures to solve non-linear systems of equations for dassl and just a failure for other solvers. Similar result as reference.

With events you also get two events very close together; which might indicate another numerical issue.

The reason to have smooth in the model is that in some other operating mode the OpAmp might change back and forth a lot, and you don't want events for that.

My estimate is that in reality such a model has some extra part (capacitance, inductance) that is missing from this model - and that extra part avoids the issue.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 21, 2024

For me it seems that the parameter {opAmp1, opAmp2}.strict (on the Advanced tab) has a great influence.
With strict = false the Dymola result is nonsense, with strict = true it seems correct to me.
With both strict = true and strict = false the OM result is not correct:
grafik
I suggest the following:

@HansOlsson
Copy link
Contributor

HansOlsson commented Oct 21, 2024

Another quick solution: Change the parameters of the idealCloser to Ron=1e-4 and Goff 1e-4 (both default 1e-5) and the example runs like a charm with tolerance 1e-6 and tolerance = 1e-7. There's only negligible deviation in the results, mainly the current before the switch is closed grows from 0.5 mA by a factor of 10. One more fact: With original parameters, the example simulates fine with OM and Dymola if you choose Algorithm = Cvode even with tolerance=1e-7. @christiankral what's your opinion, what should we do? @HansOlsson do you have an idea about the reason for this weird behavior (timeout with tolerance 1e-7)?

The problem seems to be the state imc.stator.zeroInductor.i0, increasing its nominal value to 10 avoids the issue for tolerance 1e-7 and dassl.

That signal looks like a very small sine-signal with period 0.02 s (or 50 Hz), as if it is caused by some residual current. I have not analyzed that further.

@AHaumer
Copy link
Contributor

AHaumer commented Oct 21, 2024

The problem seems to be the state imc.stator.zeroInductor.i0, increasing its nominal value to 10 avoids the issue for tolerance 1e-7 and dassl.
That signal looks like a very small sine-signal with period 0.02 s (or 50 Hz), as if it is caused by some residual current. I have not analyzed that further.
@HansOlsson indeed this signal should be zero but isn't due to numerical inaccuracy.
I'll check your solution. How would it work for other simulators?
Ok checked the specification. The nominal attribute is a common definition, and it works!
I'll prepare a PR: #4486

@casella
Copy link
Contributor

casella commented Oct 24, 2024

Regarding the first issue with Modelica.Electrical.Analog.Examples.OpAmps.SignalGenerator, opAmp1.out.v should not have any spikes at all.

How do you know that for the Modelica model?

I don't know it from the Modelica. I know that from the physics: this circuit has +/- 15 V supply, so the output voltage of the op-amp shall be within that range.

Whether this is a tool issue or a modelling issue I also don't know, but for certain resolving the issue (whatever it is) by removing the signal from the set of references is a classical dirt-under-the-carpet action I wouldn't recommend 😃

@casella
Copy link
Contributor

casella commented Dec 12, 2024

I worked a bit more on the topic, here are my findings, I'm available to discuss them in another meeting next week.

It is true that we are late with 4.1.0. On the other hand, the current SignalGenerator models clearly shows that modelling opAmps for switching applications with smooth()/noEvent() is really not a good idea in terms of robustness and portability. The problem has been there for a while, and we should finally fix that for good.

The reason is that the gain of the operational amplifiers are very high (in fact, in reality they are much higher than the gain Toni set for his examples!), and the circuit is extremely non-linear, so this may stress the solvers a bit too much.

I checked the currently implemented MSL 4.1.0-beta.1 SignalGenerator in Dymola 2024x

  • with Tolerance = 1e-6, opAmp1.v_out has those spikes at 1e37 V, which are clearly non-physical.
    u3eV1CpC0nFvFHvz

On the other hand, the triangular wave signal of opAmp2.v_out has the correct shape, and a frequency of 10.001 Hz, which is very close to the theoretical value 10 Hz

  • with Tolerance = 1e-7 some high-voltage spikes in opAmp1.v_out are missing

lO01t0iQgjfVYZhq

otherwise same result. BTW, this is the current reference trajectory for MSL 4.1.0 regression testing, which is obviously not good.

  • with Tolerance = 1e-8, the solver fails as soon as opAmp1.v_out reaches the upper threshold +10 V.
    ... Error message from dymosim
    At time T = 2.499806e-02 and stepsize
    H = 2.622482e-16 the corrector could not converge
    because there were model evaluation failures
    and the stepsize cannot be reduced further.
    Integration will be terminated.
    Integration terminated before reaching "StopTime" at T = 0.025

Changing the number of intervals in the simulation has no visible effect.

This is what happens with OpenModelica, 1.25.0-dev nightly build

  • with Tolerance = 1e-6, the square wave of opAmp1.v_out does not have spikes, but it has an irregular shape, not periodic
    AhwaunETUCe0En2Q
    The triangular wave of opAmp2 is also irregular, accordingly
    EQun4YMRlu4WjidC

  • with Tolerance = 1e-7, the situation is improved a bit

zMjEB28eMlimFl6y

though the frequency of the wave is significantly off at 10.06 Hz and the first triangular wave doesn't reach -10 V. However, if I decrease the Interval from 0.001 to 0.0002, I get this result:

swoNUdI00dH8NRxS

this is quite weird and suggests that the model is numerically challenged.

  • At Tolerance = 1e-8, I get these two results with Interval = 0.001 and Interval = 0.0002

Pb5PtP9tg3tlrN21
b9r5X1ycFUmAvban

so tightening the tolerance actualy makes things much worse. At least for some communication intervals. These are all tell-tale signs of a numerically ill-conditioned model

What is interesting is that both Dymola and OMC produce a wide zoo of badly wrong results depending on solver settings, even though the ways they are wrong are completely different. This is probably due to different implementations of the smooth() operator, which are legitimate. But in any case, to me this scenario is clearly not acceptable for a model in the Standard library.

I don't know what happens with WLS, SimulationX and MapleSim, but to me the verdict is clear: this model without events is numerically fragile and has no place in the MSL.

According to François Cellier (Continuous System Simulation, Chapter 9) this is well-known since the late 70s: relying on "soi-disant event-handling capabilities of variable step-size solvers" when you have switching circuits is not a good idea. He was actually referring to discontinuous models, but I think this also applies with continuous (but not continuously differentiable) models with high feedback gains. Proper event handling was introduced exactly to handle these cases in robust and reliable way.

@casella
Copy link
Contributor

casella commented Dec 12, 2024

Now, disregarding homotopy, which only matters for initialization, the current opAmp model has two modes of operation

  • strict = true: smooth(0, noEvent(if V0*v_in>vps then vps else if V0*v_in<vns then vns else V0*v_in)
  • strict = false: smooth(0, if V0*v_in>vps then vps else if V0*v_in<vns then vns else V0*v_in)

In the first case, events are explicitly verboten, in the second case, tools may try play some tricks with if-expressions and event-handling in the (vain) attempt of making the simulation faster but, as we've seen, the results are not necessarily successful.

I tried again Toni's new implementation in OpenModelica, by switching off all the advanced flags for both opAmps, so that the plain equation

    v_out = (if V0*v_in>vps then vps else if V0*v_in<vns then vns else V0*v_in)

is used in both, with event generation.

In order to avoid the ambiguity of the initial solution, I simply set the initial voltage of the capacitor to 10 V, which corresponds to a single solution of the initial equations.

Using OpenModelica with tolerance = 1e-6, the simulation fails numerically for amplification factors V0 > 2000 (Toni set V0 = 15000 in his model). However, if I set Tolerance = 1e-7, 1e-8, 1e-9, or even 1e-10, and change the length of the Interval betweeen 0.001 and 0.0001, the simulation always runs successfully with V0 = 15000, and always produces the correct results:

0IbeIR6qxPbZWyz4

The frequency 10 Hz of the square wave is reproduced with very high accuracy already at Tolerance = 1e-7.

The fact that a tight tolerance is necessary to get through this simulation is understandable, given the high nonlinearity of the system. At each switching, opAmp1 enters and leaves the saturation region in a very short time interval (of the order of the microsecond), due to its very high gain. So, I'm not surprised that a too sloppy tolerance causes solver failure. The important thing is that tightening it does not make things worse.

Regarding the initial conditions, I think setting c.v(start = 10) is perfect if one wants to demonstrate the periodic behaviour of the circuit, as in the current reference results. While there are three possible solutions when one sets the initial voltage at zero (due to the actual physical behaviour), there is a unique solution of the nonlinear equations in that case.

I believe that the behaviour of examples in the MSL should be non-ambiguous and do not depend on the choice of tearing variables and their start attribute, so a configuration that unambiguously determines the initial conditions only based on the initial state values should be preferred.

If one wanted to simulate what happens when you turn the circuit on, one could also set, say, c.v(start = 0.001), to emulate the effect of small voltage fluctuations affecting the capacitor due to thermal effects. In this case, the initial part of the transient is an exponentially increasing voltage trajectory, which ends up with this transient for opAmp2.v_out

2ZEZo0TTNmbqW3vG

Maybe we could keep both examples in the MSL, one as SignalGeneratorPeriodic and the other as SignalGeneratorStartUp.

@casella
Copy link
Contributor

casella commented Dec 12, 2024

Summing up, my proposal for MSL 4.1.0 at the moment is the following:

  • the current SignalGenerator example in the MSL is numerically unreliable and as such should not be part of the Standard library
  • reliable simulation of switching behaviour requires events
  • the IdealOpAmp model should also include one mode which triggers events, and this mode should be used for the SignalGenerator example
  • this mode is the safer one, so I'd argue it should be the default mode if you don't set any advanced flag, as in Toni's implementation, even though this is not backwards compatible. I'd regard this as a bug fix. Of course it should be documented in the release notes
  • whether or not it is worth to also include a regularized (we should use full names in Modelica, not abbreviated ones like regul) mode with actually smooth behaviour, I'm not sure. My fear is that with the high gains typical of op-amps, the nonlinearity is so strong that solvers could get into trouble and/or be extremely inefficient when handling the switching. I haven't experimented with that, yet.

Here is Toni's package with the modified SignalGenerator example case. OpAmps.mo

@AHaumer
Copy link
Contributor

AHaumer commented Jan 10, 2025

I'll provide a PR using regularized (instead of short "regul"), testing the critical examples with Dympla2025 and OM 1.24.3 (latest official release).
Don't miss the discussions in #4485 - here I committed my suggestion.
I propose to continue the discussion in #4485

AHaumer added a commit to AHaumer/ModelicaStandardLibrary that referenced this issue Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example Issue only addresses example(s) L: Complex* Issue addresses Complex, Modelica.ComplexBlocks or Modelica.ComplexMath L: Electrical.Machines Issue addresses Modelica.Electrical.Machines L: Electrical.PowerConverters Issue addresses Modelica.Electrical.PowerConverters L: Electrical.Spice3 Issue addresses Modelica.Electrical.Spice3 L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Projects
None yet
Development

No branches or pull requests

6 participants