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
Inclusion of MoMoLib in trunk #1105
Comments
Comment by dietmarw on 15 May 2013 14:45 UTC
The items named above shows why we normally should not throw a new library into the trunk but evaluate and improve them externally for at least 6 month. I'm really not sure why this was not done in this case :-(. |
Comment by adrpo on 16 May 2013 05:29 UTC As far as I understand it the new models are not conform to the
In the simulation model Modelica.Media.Examples.R134a.R134a1 you have
package R134a_ph contains:
Common.PhaseBoundaryProperties contains these:
which have types accessed from a partial package. So are these models conform to the specification or not? Cheers, |
Comment by otter on 16 May 2013 07:02 UTC
Library officers decide what is included in their libraries and for this no discussion is needed in MAP-LIB (otherwise this would be not practical if every minor detail is discussed in the ticket system). I am library officer of SIunits. I included three new types because they are used in new components included in Modelica.Media. The new types are included exactly that the place in SIunits where other similar types are present: MolarDensity is added at the place where MolarMass, MolarVolume, MolarEnergy is present and therefore this is the right place. DerPressureByDensity and DerPressureByTemperature are added at the place where several DerxxxByyyy are present. There is a difference whether a new type shall be included that is nowhere used in MSL (I don't do this without proper reasoning or discussion), or whether a type is introduced that is used in MSL (must be defined somewhere). |
Comment by otter on 16 May 2013 07:28 UTC
This is your opinion. A developer may have another opinion, and it depends on the actual situation whether it is more practical to keep an out-commented code part for some time.
I guess that no Modelica class in MSL follows 100 % the guidelines. Note, these are guidelines and not requirements. It is impossible to check by a human whether all parts of the guidelines are fulfilled. Therefore, a comment like "does not follow the guidelines" is quite useless. Please, point out the major issues that should be improved in the documentation
I searched, but did not find this issue. E.g. the records introduced in Media.Commons extend from Modelica.Icons.Record. Again, such an unspecific bug report is quite useless. Please, be specific and point to the issue.
New components have been included in an existing library (Media). The library officer is responsible for the content that is introduced in his library. I have coordinated this with Hubertus (the library officer) and XRG (the developers). Hubertus agreed to put it in and Hubertus and XRG discussed how to include it and how to rename the classes. I was just implementing the result of this discussion because neither XRG nor Hubertus had currently time to work on this. The MoMoLibcomponents have been added to the development branch in Jan 2013, and since then several improvements have been made on the development version. Finally, everything seemed to be o.k. and Pedantic Check with Dymola and Check with Simulation (with exception of one model) was successful. Furthermore, in the CleanSky project the functionality of the library was carefully evaluated. It then seemed ready to include the library in trunk. I don't understand why you use the term "throw a library in the trunk" in such a situation. |
Comment by otter on 16 May 2013 07:36 UTC
The definition of Common.PhaseBoundaryProperties in the trunk is:
You seemed to point to the wrong code? Please, can you clarify.
|
Comment by sjoelund.se on 16 May 2013 07:54 UTC |
Comment by adrpo on 16 May 2013 07:59 UTC I think you're mistaken. This is the code in trunk Modelica/Media/R134a.mo, ba2f041.
Cheers, |
Comment by dietmarw on 16 May 2013 08:02 UTC
We are talking about a new component not a changed one where it might still be valuable to keep old code-snippets. The release version of MSL should not serve as a container for out-commented development ideas this is what branches can be used for.
Since some time we are struggling to improve the quality of the MSL. True the current MSL does not follow the guidelines 100%, that's why we said that all that is added to it follows this so that we only need to worry about updating existing code. It's much easier to fix new things at once than to go over it after some time. You say it is impossible by a human to follow the guidelines? They are not that much to read really and also quite easy to follow. If we can not manage even that tiny bit then I really don't want to think about what else we don't follow when adding new models. In particular I meant that:
Yes sorry, this was actually Dymola not displaying the icons correctly in the browser view.
Well this ticket is the first one filed on the inclusion of MoMoLib. This should have happened back then when it actually came up for discussion. Only until some days ago it was not clear that it should be included as of now. So people/tool vendors didn't even have a chance to evaluate it properly. So even though it was available on some development branch does not automatically mean is going to be included right? It really has to do with having a proper transparent procedure in place so that people can give feedback on possible things that needs fixing BEFORE it goes into the trunk. This was unfortunately not the case. I'm rather careful with trusting statements like "in the CleanSky project the functionality of the library was carefully evaluated" as long as the details of the evaluation are not available. E.g., was it done with different tools or just one and are the tools following the Modelica Specification. Since MSL 3.2.1 must be 100% standard this is important. And checking is of much value. Also I could not see any test models being committed for the new Media types. I'm not against inclusion in general just a certain QA should be followed or we end up with similar problems like after the premature inclusion of Spice3. |
Comment by otter on 16 May 2013 08:24 UTC
Fixed in 88fe477. |
Comment by adrpo on 16 May 2013 12:35 UTC Thanks Martin O. Cheers, |
Comment by leo.gall on 16 May 2013 12:39 UTC
I attached a report created by Dymola ModelManagement to this ticket. So it's clear, which description strings should be adapted. I volunteer to make the trivial fixes, by changing lower case to capital letters (probably next week). |
Comment by sjoelund.se on 16 May 2013 12:45 UTC It would be better to make a stable release, then add the proposed libraries and fix them for the next release. I am fairly certain all quirks will not be sorted out before the release (Adrian founds bugs within a few minutes of looking at the library despite it being checked with Dymola Pendantic). |
Comment by adrpo on 16 May 2013 12:46 UTC I have another issue, this time with model:
You have in
Then Modelica.Media.Air.RealGasMoistAir contains:
However Cheers, |
Comment by otter on 16 May 2013 12:53 UTC
Yes, please do it. I recognized that in the whole Modelica.Media library there are many descriptions strings that start with a lower case letters. Maybe, you can correct them as well. |
Comment by adrpo on 16 May 2013 12:53 UTC |
Comment by otter on 16 May 2013 13:05 UTC
This is a misunderstanding. No new library is added. Instead three new medium models are added to the library Modelica.Media. These are just media models as all the others. The big issue for MSL 3.2.1 was that Modelica.Media was not fully compliant to the Modelica specification and it took some effort to make this compliant to each other and still backwards compatible. The newly introduced media have been adapted to this slightly changed library structuring, and there should be no problem. There are some compliance issues that are not reported by Dymola pedantic mode. Just run your tools to detect these and this can be quickly fixed.
How do you know that there are no problems with all the other media in Modelica.Media? This is a huge library and the only way to find this out is that a tool analysis the library and reports problems, and this can be performed within a few minutes. So, just run the same (automatic) tests again on Modelica.Media as run before and thats it (I run the once where I have easily access to it). Note, there are still about 60 new components in the trunk (probably more). We do not yet have a feature freeze, and there was never a decision to add NO new component to MSL 3.2.1 |
Comment by adrpo on 16 May 2013 14:29 UTC
and
And There are plenty of these patterns in Media, see also #1106 (not as obvious as this one). I have nothing against using stuff from partial packages directly via qualified names but the specification seems not to allow it. Cheers, |
Comment by wischhusen on 17 May 2013 11:18 UTC I had a look at the HTML issues. Here is the result for Modelica.Media: In almost 10000 lines there are 12 lines with warnings for Modelica.Media. My hypothesis from the warning found
is that the check routine does not verfiy upper case HTML commands. In fact the command " used in Modelica.Media is right when a tool is not case sensitive. There are plenty of warnings like that for other parts of MSL. If I am right the solution for Modelica.Media would be to change
into
Probably it is best to test it. If it resolves the problem it should be mentioned in the HTML guidelines that one has to use lower case commands. Thanks to Leo for adding capital letters in the description strings.
|
Comment by sjoelund.se on 17 May 2013 11:33 UTC |
Comment by otter on 17 May 2013 13:53 UTC
I fixed the html-errors already yesterday. |
Comment by dietmarw on 17 May 2013 15:10 UTC
The HTML errors that are reported by tidy.filtered are not just for the MSL but for all public libraries as listed on https://modelica.org/libraries. Hence the large number of warnings. Warnings for MSL should always be kept at 0. |
Comment by hubertus on 17 May 2013 17:46 UTC
Which is non-partial. If you use basic types and composite ones like FluidConstants from there, everything will be conformant to the specification. I will move this issue to #1106, and assign it to you, Stefan. |
Comment by hubertus on 17 May 2013 18:26 UTC
Sorry that I did not have time to review this earlier. |
Comment by hubertus on 17 May 2013 19:16 UTC I would strongly suggest to change the name before we actually make the inclusion. |
Comment by dietmarw on 17 May 2013 20:22 UTC |
Comment by otter on 18 May 2013 10:02 UTC
Fixed in d1080b7 by changing the name to ReferenceAir. Question: Should the RealGasMoistAir name then also be changed, say, to ReferenceMoistAir? |
Comment by hubertus on 18 May 2013 18:39 UTC |
Comment by otter on 20 May 2013 09:14 UTC
Fixed in b9335eb by providing the corrected copyright notice. |
Comment by otter on 20 May 2013 10:04 UTC
Fixed in c758397, f6cb1f0, f8066f7: Removed the definitions of NewtonDerivatives_ph, NewtonDerivatives_ps, HelmholtzDerivs, Helmholtz_ph in R134a and replaced references by the appropriate Media.Commons.XXX element. The elements cv2Phase, helmholtzToBoundaryProps, and PhaseBoundaryProperties have the same names as in Media.Commons, but they have more elements and can therefore not be removed |
Comment by wischhusen on 20 May 2013 18:59 UTC
Done with 920f38b. |
Comment by leo.gall on 21 May 2013 09:22 UTC |
Comment by otter on 21 May 2013 09:56 UTC |
Comment by dietmarw on 22 May 2013 13:06 UTC
|
Comment by otter on 22 May 2013 13:20 UTC |
Reported by dietmarw on 15 May 2013 14:22 UTC
In ba2f041 the MoMoLib was included in trunk. This ticket should discuss possible issues that need to be resolved before the general release.
Migrated-From: https://trac.modelica.org/Modelica/ticket/1105
The text was updated successfully, but these errors were encountered: