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)
Change History (28)
comment:1 by , 4 years ago
Priority: | critical → blocker |
---|
comment:2 by , 4 years ago
Cc: | added |
---|
follow-up: 5 comment:3 by , 4 years ago
Status: | new → accepted |
---|
by , 4 years ago
Attachment: | TestReplaceableMedia.mo added |
---|
comment:5 by , 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 , 4 years ago
Proper models now (actually subtypeof) with: https://github.com/OpenModelica/OpenModelica/pull/6765
Needs to be filtered more.
comment:7 by , 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.
comment:8 by , 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.
follow-up: 10 comment:9 by , 4 years ago
PR: https://github.com/OpenModelica/OpenModelica/pull/6777
hides the spurious messages.
We leave this open until we fix the filtering.
comment:10 by , 4 years ago
Replying to adrpo:
PR: https://github.com/OpenModelica/OpenModelica/pull/6777
hides the spurious messages.
Looks good.
follow-up: 16 comment:12 by , 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
by , 4 years ago
Attachment: | TestMatching.mo added |
---|
by , 4 years ago
Attachment: | DropDown.png added |
---|
comment:13 by , 4 years ago
Cc: | added |
---|
@adrpo, @perost, please see the Modelica Specification Issue #2693 and discussion therein.
comment:14 by , 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.
follow-up: 19 comment:15 by , 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.
comment:16 by , 4 years ago
Replying to Andrea.Bartolini:
It seems that another similar issue is still present...
Open the modelM3
in the attached packageTestMatching.mo
, open theM2
model parameters pop-up window and click on thermod
drop-down list: together withD1
andD2
models (that are correctly shown) also thermod
models, that have modelD1
declared as default model in the replaceable statement inside the modelsM1
andM2
, 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.
follow-up: 18 comment:17 by , 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)'
comment:18 by , 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.
comment:19 by , 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:21 by , 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.
Why partial packages cannot be used? Via extends they become non-partial.