Opened 4 years ago

Last modified 3 years ago

#6129 accepted defect

choicesAllMatching doesn't return the appropriate class references

Reported by: Francesco Casella Owned by: Adrian Pop
Priority: blocker Milestone: 1.19.0
Component: Interactive Environment Version:
Keywords: Cc: Andrea Bartolini, Per Östlund

Description

Please check the attached simple model, I just dragged a pressure source component boundary in it.

When I double-click on boundary, and then click on the Medium drop-down menu, I get about 30 possible classes for redeclarations. However, there are several issues there

  • some are partial packages, which obviously cannot be used, e.g., Modelica.Media.Interfaces.TemplateMedium
  • some are found within models but are not encapsulated, so according to Section 5.3.2 of the specification, they shouldn't even be looked up, e.g. Modelica.Media.Examples.Tests.Components.ShortPipe.Medium
  • some are even found inside partial models, e.g. Modelica.Media.Examples.Tests.Components.PartialTestModel.Medium

On the other hand, not even one of the actually usable models, e.g. Modelica.Media.Air.SimpleAir or Modelica.Media.Water.StandardWater actually show up.

This issue makes OMEdit actually unusable with Modelica Media/Fluid

Attachments (3)

TestReplaceableMedia.mo (295 bytes ) - added by Francesco Casella 4 years ago.
TestMatching.mo (599 bytes ) - added by Andrea Bartolini 4 years ago.
DropDown.png (23.5 KB ) - added by Andrea Bartolini 4 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 by Francesco Casella, 4 years ago

Priority: criticalblocker

comment:2 by Francesco Casella, 4 years ago

Cc: Andrea Bartolini added

comment:3 by Adrian Pop, 4 years ago

Status: newaccepted

Why partial packages cannot be used? Via extends they become non-partial.

by Francesco Casella, 4 years ago

Attachment: TestReplaceableMedia.mo added

comment:4 by Francesco Casella, 4 years ago

Sorry, I forgot the attachment

in reply to:  3 comment:5 by Francesco Casella, 4 years ago

Replying to adrpo:

Why partial packages cannot be used? Via extends they become non-partial.

There is no extends clause in the attached model. I'm not sure what the specification says (I can check), but for sure it doesn't make sense to redeclare a medium model to a partial one, because then there will be some missing stuff and you will not be able to compile the model and run it.

Maybe you could do that within a partial model, but normally you don't use the GUI to do this kind of things. In any case, the attached model is not partial, so it is supposed to work, and hence it cannot use a partial medium package.

Do I miss something?

comment:6 by Adrian Pop, 4 years ago

Proper models now (actually subtypeof) with: https://github.com/OpenModelica/OpenModelica/pull/6765
Needs to be filtered more.

Last edited 4 years ago by Adrian Pop (previous) (diff)

comment:7 by Francesco Casella, 4 years ago

I tested the results of the PR today with the attached test case. Now all the relevant packages show up. However, there are still some more that should not belong there and should be filtered out

Partial classes, e.g. Modelica.Media.Interfaces.PartialPureSubstance, that would produce an error when the redeclare statement is later processed by the front end, should be removed.

In general, all names that include at least one partial class should be filtered out, since lookup in partial classes is not allowed in a simulation model.

If I am not mistaken, applying this simple rule should allow us to get rid of all the unwanted entries in the drop-down menu.

Last edited 4 years ago by Adrian Pop (previous) (diff)

comment:8 by Francesco Casella, 4 years ago

There are also many red error messages showing up in the Message Browser, such as

[1] 11:16:25 Translation Error
[Modelica.Media: 6555:7-6559:22]: Called function ‘setState_pTX‘ is partial.

[2] 11:16:25 Translation Error
Class Modelica.Media.Interfaces.PartialTwoPhaseMedium.density_pT not found in scope <top>.

[3] 11:16:25 Translation Error
Class Modelica.Media.Interfaces.PartialRealCondensingGases.specificEntropy_phX not found in scope <top>.

[4] 11:16:25 Translation Error
Class Modelica.Media.Interfaces.PartialCondensingGases.enthalpyOfNonCondensingGas not found in scope <top>.

[5] 11:16:25 Translation Error
Class Modelica.Media.Interfaces.PartialMixtureMedium.massToMoleFractions not found in scope <top>.

[6] 11:16:25 Translation Error
Class Modelica.Media.Interfaces.PartialPureSubstance.density_pT not found in scope <top>.

which should be suppressed if -d=nfAPINoise is not activated.

comment:9 by Adrian Pop, 4 years ago

PR: https://github.com/OpenModelica/OpenModelica/pull/6777
hides the spurious messages.

We leave this open until we fix the filtering.

in reply to:  9 comment:10 by Francesco Casella, 4 years ago

Replying to adrpo:

PR: https://github.com/OpenModelica/OpenModelica/pull/6777
hides the spurious messages.

Looks good.

comment:11 by Andrea Bartolini, 4 years ago

Seems to work on Ubuntu 18.04

comment:12 by Andrea Bartolini, 4 years ago

It seems that another similar issue is still present...
Open the model M3 in the attached package TestMatching.mo, open the M2 model parameters pop-up window and click on the rmod drop-down list: together with D1 and D2 models (that are correctly shown) also the rmod models, that have model D1 declared as default model in the replaceable statement inside the models M1 and M2, are shown (see attached DropDown.png image).

OMEdit - OpenModelica Connection Editor
Connected to OpenModelica 1.17.0~dev-22-gf1438db
Sysop: Ubuntu 18.04

Last edited 4 years ago by Andrea Bartolini (previous) (diff)

by Andrea Bartolini, 4 years ago

Attachment: TestMatching.mo added

by Andrea Bartolini, 4 years ago

Attachment: DropDown.png added

comment:13 by Francesco Casella, 4 years ago

Cc: Per Östlund added

@adrpo, @perost, please see the Modelica Specification Issue #2693 and discussion therein.

comment:14 by Francesco Casella, 4 years ago

After the last commits by @adrpo, the original issue is solved, I no longer see partial classes in the drop-down menu.

Also, using the string-based method made the whole thing a lot faster, you don't have to wait 5-10 seconds for the drop-down menu to materialize.

I still see classes defined in models contained in packages, e.g. Modelica.Fluid.Fittings.MultiPort.Medium. Whethere or not we want to see them is questionable, see the discussion in MSL issue #2693, but as long as their use is legal, I'm fine with that at least for this version.

In fact Modelica.Fluid.Fittings.MultiPort.Medium should not be there because that Medium is actually declared as Modelica.Media.Interfaces.PartialMedium in the source code, so it is a partial package that cannot actually be used for a simulation model.

@adrpo, do you think fixing that is easy? Otherwise we can do that for 1.17.0, that's not such a big deal.

comment:15 by Adrian Pop, 4 years ago

Is not the string method that is used, is just a heuristic to detect what stuff is already fully qualified.

I guess short class definitions from partial packages (especially those that do not have *any* modifiers) should be filtered out too, should be easy to handle. I'll try to fix this for 1.16 final.

in reply to:  12 comment:16 by Francesco Casella, 4 years ago

Replying to Andrea.Bartolini:

It seems that another similar issue is still present...
Open the model M3 in the attached package TestMatching.mo, open the M2 model parameters pop-up window and click on the rmod drop-down list: together with D1 and D2 models (that are correctly shown) also the rmod models, that have model D1 declared as default model in the replaceable statement inside the models M1 and M2, are shown (see attached DropDown.png image).

After the last commits, the D1 model is no longer shown in the drop-down list. Why was it filtered out?. It is true that the default value is already D1, but one may want to explicitly redeclare it to that value, and this is currently not possible. @adrpo, could you get it back for the final release?

Regarding the other two choices, whether or not should show them have them is the subject of #2693, we may keep it like it is now for the 1.16.0 release, and then see how that discussion goes.

Last edited 4 years ago by Francesco Casella (previous) (diff)

comment:17 by Adrian Pop, 4 years ago

Sure, we can bring it back, @adeas31 and me thought it would be better to filter out the default from the dropdown.

As one can see from the call to getAllSubtypeOf the TestMatching.D1 is there :)

getAllSubtypeOf(TestMatching.B1,TestMatching.M2,false,false,false) 00:09:59:173
{TestMatching.M2.rmod,TestMatching.M1.rmod,TestMatching.D2,TestMatching.D1} 00:10:01:682
#s#; 2.509000; 11.340000; 'getAllSubtypeOf(TestMatching.B1,TestMatching.M2,false,false,false)'
Last edited 4 years ago by Adrian Pop (previous) (diff)

in reply to:  17 comment:18 by Francesco Casella, 4 years ago

Replying to adrpo:

Sure, we can bring it back, @adeas31 and me thought it would be better to filter out the default from the dropdown.

I would rather keep it for consistency with other cases of drop-down, e.g. Booleans or enumerations. In all those cases, you can always explicitly set the value to the default value. I don't see why replaceable classes should behave differently.

in reply to:  15 comment:19 by Francesco Casella, 4 years ago

Replying to adrpo:

I guess short class definitions from partial packages (especially those that do not have *any* modifiers) should be filtered out too, should be easy to handle.

I agree.

I'll try to fix this for 1.16 final.

Only if it doesn't take too much time :)

comment:20 by Adrian Pop, 4 years ago

Do not filter the default fixed via PR:

https://github.com/OpenModelica/OpenModelica/pull/6856

comment:21 by Adrian Pop, 4 years ago

The short class definition filtering would be possible but it would slow things down a bit. I'll see what I can do until Monday.

comment:22 by Francesco Casella, 4 years ago

Milestone: 1.16.01.17.0

Retargeted to 1.17.0 after 1.16.0 release

comment:23 by Francesco Casella, 4 years ago

Milestone: 1.17.01.18.0

Rescheduled to 1.18.0

comment:24 by Francesco Casella, 3 years ago

Milestone: 1.18.0

Ticket retargeted after milestone closed

comment:25 by Francesco Casella, 3 years ago

Milestone: 1.19.0

1.18.0 blocker tickets moved to 1.19.0

Note: See TracTickets for help on using tickets.