search

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
467 stars 166 forks source link

Garbage in a lot of annotations in MSL 3.2 and MSL 3.1. #457

Closed modelica-trac-importer closed 7 years ago

modelica-trac-importer commented 7 years ago

Reported by adrpo on 17 Nov 2010 12:17 UTC Just search for "extent=[" and you will find plenty of this garbage. The problem is that this garbage makes annotations not correct. Example: Modelica.Electrical.Analog.Basic.M_Transformer:

annotation (extent=[-80,-40; -62,40], Placement(transformation(
          extent={{-80,-40},{-62,40}}, rotation=0)));
or below, in:
annotation(Icon(..., graphics={...},
      Ellipse(extent=[-36,24; -18,42]),
      Ellipse(extent=[18,24; 36,42]),
      Ellipse(extent=[0,24; 18,42]),
      Ellipse(extent=[-18,24; 0,42]),
...);

However, Icon annotation CANNOT contain Ellipses, they can only be part of the graphics component.

There are 353 occurrences of such garbage, see the attached file.

Migrated-From: https://trac.modelica.org/Modelica/ticket/457

modelica-trac-importer commented 7 years ago

Comment by AHaumer on 29 Nov 2010 13:42 UTC The "garbage occurence" in Magnetic.FluxTubes, Line 4 752:

            Text(
              extent=[-150,120; 150,80],
              textString="%name",
              lineColor={0,0,255}),

seems to be fine? The "extent" is placed within the "Text". It should be allowed to use either [-150,120; 150,80] or {{-150,120}, {150,80}}.

modelica-trac-importer commented 7 years ago

Modified by AHaumer on 29 Nov 2010 13:43 UTC

modelica-trac-importer commented 7 years ago

Comment by adrpo on 6 Dec 2010 15:33 UTC Yes, extent=[-150,120; 150,80] should be fine if is in the proper place (inside a graphical component). Sorry about these "false positives".

Cheers, Adrian Pop/

modelica-trac-importer commented 7 years ago

Modified by dietmarw on 24 Nov 2011 13:46 UTC

modelica-trac-importer commented 7 years ago

Modified by dietmarw on 19 Jan 2012 14:17 UTC

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 22 May 2012 11:00 UTC Adrian, would it be possible for you to regenerate the GarbageInAnnotations.txt file since some of the garbage might have been removed by ffbcd7378b619210bec7cb2e83e84a47e2ec8c3c.

modelica-trac-importer commented 7 years ago

Comment by adrpo on 22 May 2012 11:23 UTC

Hi,

To get your answer just do fgrep in the top Modelica trunk working copy directory:

adrpo@ida-liu050 ~/dev/Modelica/Modelica
$ fgrep -r -n "extent=[" * | grep -v "\.html" | less
...
$ fgrep -r -n "extent=[" * | grep -v "\.html" | wc -l 
340

Seems there are still some left, look for extent appearing directly in annotations :) You might get some false positives via grep as some of them appear inside graphical annotations.

Cheers, Adrian Pop/

modelica-trac-importer commented 7 years ago

Comment by pharman on 22 May 2012 15:28 UTC I've also attached a file with some errors found in annotations in the MSL r5133, this includes the additional extent annotations but also others, such as:

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 22 May 2012 15:33 UTC We should try really hard this time to get a MSL which follows the specification in all parts.

modelica-trac-importer commented 7 years ago

Comment by clauss on 23 May 2012 06:15 UTC Sorry, I see no practicable way to clean this garbage in a short time:

modelica-trac-importer commented 7 years ago

Comment by sjoelund.se on 23 May 2012 06:47 UTC Of course you can guarantee no further garbage is inserted: Review the diff before you commit to verify that nothing odd was changed.

For simple changes: edit the file in notepad and only use Dymola for testing.

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 24 May 2012 15:03 UTC Checking the diff is good for many purposes; in particular it detects the case where you for testing did some change that you longer need. I would guess that the multiple conflicting annotations is due to merging in svn and would be detected by reviewing the diffs.

The 'extent' annotations were introduced a long time ago when graphical annotations were converted to Modelica standard graphical annotations; a short time the old annotations were also kept.

The Error-annotations are only introduced when you switch from a class with syntax errors in it. InlineNoEvent and NumberOfIntervals are added in some form in Modelica 3.3 with other names (and different semantics for NumberOfIntervals).

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 24 May 2012 15:23 UTC Replying to [comment:12 hansolsson]:

The Error-annotations are only introduced when you switch from a class with syntax errors in it.

But surely Dymola should remove those again (and call them __Dymola_Error) once the class syntax is fine again.

InlineNoEvent and NumberOfIntervals are added in some form in Modelica 3.3 with other names (and different semantics for NumberOfIntervals).

OK going to rename those two (or remove if not needed) to __Dymola_... and added a two tickets to handle the migration to the standard solutions as defined in the Spec 3.3 (#747 and #748)

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 25 May 2012 08:10 UTC Replying to [comment:13 dietmarw]:

Replying to [comment:12 hansolsson]:

The Error-annotations are only introduced when you switch from a class with syntax errors in it. But surely Dymola should remove those again (and call them __Dymola_Error) once the class syntax is fine again.

Oh, I realized that the cause was different:

The "Error" tag is as far as I can understand introduced when some parsing issues are found (and of course reported); basically there was "garbage"/incorrect text in the input file and "Error" was introduced to not stop the parsing of the rest of the text. If the error had been corrected and reparsed the "Error" tag would be gone.

The problem is the lost incorrect text - and not the "Error"-tag.

The tag for keeping syntax error in the text-editor is actually called something else (and I agree that it should be cleared out once the error is corrected).

modelica-trac-importer commented 7 years ago

Modified by dietmarw on 25 May 2012 15:16 UTC

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 25 May 2012 15:53 UTC OK I've fixed the class annotation errors in 1605e983a84724abf77651895235c27992a4409d : 804b46e7ea8150d29eaffde0e320fc50ab51e0c0 for now. Adrian, could you rerun your check again so I have a somewhat reduced log to work on in order to fix the component annotations later?

modelica-trac-importer commented 7 years ago

Comment by adrpo on 25 May 2012 16:01 UTC

Done. GarbageInAnnotations-2012-05-25.txt is attached now. Obtained via:

$ fgrep -r -n "extent=[" * | grep -v "\.html" | grep -v "\.txt" > GarbageInAnnotations-2012-05-25.txt

Cheers, Adrian Pop/

modelica-trac-importer commented 7 years ago

Comment by ahaumer on 28 May 2012 11:42 UTC Am I wrong that all garbage left is of the same type? extent=[r,r; r,r] instead of extent={{r,r},{r,r}} Aren't both valid (see spec 3.2 exmaple on top of page 196, chapter 17.5.7.1)? Except one case: Modelica/StateGraph.mo:3036: annotation(extent=[-10,-10; 10,10]); I'll correct that funny thing.

modelica-trac-importer commented 7 years ago

Comment by ahaumer on 28 May 2012 11:46 UTC Modelica/StateGraph.mo:3036: annotation(extent=[-10,-10; 10,10]); isn't a false-positive since it is inside Documentation(info=" ... ")

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 28 May 2012 11:54 UTC Yes; the extent=[r,r;r,r] and extent={{r,r},{r,r}} are both valid (assuming they are found at the right places - otherwise both are invalid). For technical reasons it is likely that most incorrect annotations are of the form extent=[...] - but no guarantee one way or the other.

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 28 May 2012 13:46 UTC Toni, as Hans pointed out, they are legal if put at the right place. In case of the reported annotations those must be within a Placement statement which they currently are not. Hence this needs to be fixed.

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 28 May 2012 16:36 UTC Replying to [comment:21 dietmarw]:

Toni, as Hans pointed out, they are legal if put at the right place. In case of the reported annotations those must be within a Placement statement which they currently are not. Hence this needs to be fixed.

Many of the reported ones are actually correct, e.g. the ones in robotr3.mo are as far I can see correct (as well as the one Toni noted in FluxTubes); and should not be changed.

Basically using regular expressions to find syntax errors is using the wrong approach. A proper way is to structurally search for incorrect annotations in some way.

In an old version of Modelica.Mechanics.MultiBody I only found WorldForceAndTorque being messed up (CoordSys, multiple Documentation, graphical primitives outside of scope, ...), and color in Torus and VoluminousWheel having conflicting values for enable (hopefully those ones are now corrected since they were not found in Pete's list).

So looking at the attachment GarbageInAnnotations it has 323 lines - but many are about ModelicaTest and not as high priority.

And then we remove the false positives in MBS (at least they all look similar and the first one was correct) and we are left with 8 possibly relevant lines: The first 7 lines+line 32(StateGraph) that need to be investigated, i.e. 8 out of 323 lines are relevant.

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 29 May 2012 14:57 UTC The following comment from Hans Olsson was copied from the (now closed as duplicate) ticket #753:

There are a handful incorrect annotations remaining in MSL.

Listed here:

Most seem to be due to incorrect textual editing - some likely due to merge-handling in svn. In particular this is the case for Icon/Diagram annotations on equations and components.

Note: The following is not tested:

The attached [attachment:warningLog.txt:ticket:753 warningLog.txt] also contains a number of other reported issues.

All the annotation-issues can be found by searching for 'the annotation' and apart from the ones at the beginning there are two in in FluidDissipation (Dialog-annotation on extends-clause; which is an interesting idea).

Note: The other issues included in the [attachment:warningLog.txt:ticket:753 warningLog.txt] are found in #639, #640 and #731

modelica-trac-importer commented 7 years ago

Modified by otter on 29 May 2012 21:14 UTC

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 30 May 2012 11:39 UTC The annotation problems reported by [attachment:warningLog.txt:ticket:753 warningLog.txt] are fixed in f994d5b52ce0f8d88654b9dddf6a90a5702ea29c except for:

modelica-trac-importer commented 7 years ago

Comment by hansolsson on 30 May 2012 11:57 UTC Replying to [comment:26 dietmarw]:

The annotation problems reported by [attachment:warningLog.txt:ticket:753 warningLog.txt] are fixed in f994d5b52ce0f8d88654b9dddf6a90a5702ea29c except for: * Modelica.Electrical.Analog.Basic.M_Transformer component p the annotation 'extent' is unknown; just remove old one. * Modelica.Electrical.Analog.Basic.M_Transformer component n the annotation 'extent' is unknown; just remove old one. Which seem to be false positives. At least I could not find anything wrong with the annotations there.

I don't see the exception: the two above are also included in the change-set (first item).

modelica-trac-importer commented 7 years ago

Comment by dietmarw on 30 May 2012 13:00 UTC Replying to [comment:27 hansolsson]:

I don't see the exception: the two above are also included in the change-set (first item).

Yes you are right. I somehow forgot that I fixed that earlier. Annotations in ModelicaTest are now also fixed (8ee8a2c201e67f3e38aa811b1cb83bdad070bfcc).

So I "think" that all annotations should now be fine. Only danger is that there are still some in some out-commented classes (see #754) which I did not fix.

modelica-trac-importer commented 7 years ago

Modified by dietmarw on 2 Nov 2012 09:08 UTC

modelica-trac-importer commented 7 years ago

Changelog removed by dietmarw on 2 Nov 2012 09:08 UTC

modelica-trac-importer commented 7 years ago

Modified by dietmarw on 2 Nov 2012 09:09 UTC