ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Reference result for Modelica.Electrical.Analog.Examples.AD_DA_conversion lacks periodicity

Open qlambert-pro opened this issue 8 years ago • 16 comments

The example represents a model of a conversion of a sine wave from analog to digital and back to analog. Since the period of the sine wave is a multiple of the sampling period used for the AD conversion, I believe the output of the conversion should be periodic and have the same period as the sine wave. This property should also be shared by the output of the DA conversion.

The reference file, however, does not display it.

The values of several variables between time 0.15 and 0.151 differs from their values between time 0.05 and 0.051. Affected variables include: aD_Converter.y, dA_Converter.vout and dA_Converter.y.

qlambert-pro avatar Oct 10 '17 07:10 qlambert-pro

Where is this reference result? I could not see anything odd when running the model.

HansOlsson avatar Oct 10 '17 07:10 HansOlsson

Here is the reference, I downloaded it from "modelica.org:/files/RegressionTesting/ReferenceResults/MSL" using the instructions available at: https://svn.modelica.org/projects/RegressionTesting/AccessInfo/read-access/README.html This is the v3.2.2+build.0-beta.3 version of the baseline: AD_DA_conversion.zip

qlambert-pro avatar Oct 10 '17 07:10 qlambert-pro

@HansOlsson This either looks like a tool issue (independent of #2282, i.e. doublePrecision=true) or the signals are not suitable for comparison.

At time 0.05 and 0.15 dA_Converter.y changes discontinuously to slightly different values in Dymola 2018:

0.05,67
0.05,64
...
0.15,67
0.15,63

In comparison MWorks.Sysplorer 2017:

0.05,67
0.05,63
...
0.15,67
0.15,63

and SimulationX 3.8.4:

0.05,67
0.05,64
...
0.15,67
0.15,64

AD_DA_conversion.zip

beutlich avatar Oct 14 '17 10:10 beutlich

I cannot reproduce this in Dymola 2018.

The model is, however, very sensitive and here value to digitize is 5.0/10.0 and any slight round-off error in 5.0 can push it below the discretization limit causing the result to be 63 - or above that limit causing it to be 64.

Thus this signal is not suitable for comparison at this accuracy at this point; both 63 and 64 are acceptable answers, and this round-off can occur differently for different times.

HansOlsson avatar Oct 16 '17 15:10 HansOlsson

Should this really have been closed before we have an updated reference result?

sjoelund avatar Oct 16 '17 16:10 sjoelund

@sjoelund You mean by removing aD_Converter.y, dA_Converter.vout and dA_Converter.y from the reference results?

beutlich avatar Oct 17 '17 06:10 beutlich

Yes, if as @HansOlsson says those signals are not good for comparison, they should be removed from the reference file so people don't get more false negatives from comparing those results.

sjoelund avatar Oct 17 '17 06:10 sjoelund

Yes, if as @HansOlsson says those signals are not good for comparison, they should be removed from the reference file so people don't get more false negatives from comparing those results.

Well, the signal is generally ok so removing it altogether seems excessive - it's just two integer values that can be one of two nearby values. I don't know exactly how detailed the filtering of references can be.

If it were a pure test-example we could shift time (shiftVoltage.phase=...) to avoid this issue - but for a an Example-model that would look odd.

HansOlsson avatar Oct 17 '17 06:10 HansOlsson

I fully agree with @sjoelund that this cannot be closed without first removing the signal from the reference result (or that it is solved by shifting the phase in the model along with an updated reference result). Removing it from the list of comparison signals is not the same as somehow removing the variable from the example.

tidde avatar Oct 17 '17 08:10 tidde

Sorry, that last comment was by me, signed in to the wrong account.

henrikt-ma avatar Oct 17 '17 09:10 henrikt-ma

I fully agree with @sjoelund that this cannot be closed without first removing the signal from the reference result (or that it is solved by shifting the phase in the model along with an updated reference result).

True, but there are two reference results: the mat-file being the result of simulation, and the csv-result used for regression testing by e.g. @GallLeo - this signal should at most only be removed in the latter; and I don't think that those results are on GitHub yet (planned to be on git-lfs I think).

I am not sure if you can just un-tighten the tolerance for a specific variable instead.

HansOlsson avatar Oct 17 '17 14:10 HansOlsson

Well, even if the results are only in rsync, we should at least fix those files. Perhaps in a new build of MSL, perhaps in the current location. It's mainly comparisonSignals.txt that should be updated and then the csv-file (removing a column or several; ideally done by the library officer)

sjoelund avatar Oct 17 '17 20:10 sjoelund

Removed aD_Converter.y, dA_Converter.vout and dA_Converter.y from the list of comparison signals.

Would be better to be able to define a specific tolerance per variable for comparison.

Possible workaround: Add a low-pass-filter, so that we have a continuous variable for comparison. This should also be able to check if the AD-DA chain works as expected.

Model: grafik

model AD_DA_conversion
  extends Modelica.Electrical.Analog.Examples.AD_DA_conversion;

  Modelica.Electrical.Analog.Sensors.VoltageSensor voltageSensor annotation (Placement(transformation(
        extent={{10,-10},{-10,10}},
        rotation=90,
        origin={72,2})));
  Modelica.Blocks.Continuous.LowpassButterworth lowpassButterworth(f=100) annotation (Placement(transformation(extent={{90,-8},{110,12}})));

equation 
  connect(voltageSensor.v, lowpassButterworth.u) annotation (Line(points={{83,2},{88,2}}, color={0,0,127}));
  connect(dA_Converter.p, voltageSensor.p) annotation (Line(points={{44,16},{72,16},{72,12}}, color={0,0,255}));
  connect(dA_Converter.n, voltageSensor.n) annotation (Line(points={{44,-10},{72,-10},{72,-8}}, color={0,0,255}));

  annotation (experiment(StopTime=0.2), uses(Modelica(version="3.2.3")));

end AD_DA_conversion;

Results: grafik

@kristinmajetta what do you think of this idea?

Ticket should be closed after finding better comparison variable.

GallLeo avatar Aug 27 '18 08:08 GallLeo

Would be better to be able to define a specific tolerance per variable for comparison.

Setting comparison tolerances seems like a completely separate question. In our tests, we do have this possibility, allowing us to work around problems like this without having to exclude the variable, but I doubt that it would be worth the effort to standardize a system for specifying this.

Instead, I want the reference results to be as accurate and reliable as possible, so that we can leave it to all tool vendors to choose a test tolerance that is matched to the solver and solver settings they use for their tests.

henrikt-ma avatar Aug 28 '18 09:08 henrikt-ma

@kristinmajetta what do you think of this idea?

Reassigning and rescheduling milestone.

beutlich avatar Nov 22 '18 06:11 beutlich

yes, let's do so

kristinmajetta avatar Nov 22 '18 14:11 kristinmajetta