OpenModelica / BioChem

BioChem is a package for biochemical modeling and simulation with Modelica
7 stars 12 forks source link

Physical units #23

Closed AtiyahElsheikh closed 3 years ago

AtiyahElsheikh commented 3 years ago

Improvements for the use of annotations for physical units. The idea is annotations should be all moved to the Icons package.

AtiyahElsheikh commented 3 years ago

Only the last two commits to review

sjoelund commented 3 years ago

I guess with these changes, there should technically be a conversion script added to rename icons (and the next release would be a major release).

AtiyahElsheikh commented 3 years ago

I did not rename the Icons. There were only three Icons in version 1.0.2 and they are still employed (Except IconBase) which does not seem to be used. All other Icons have been added to occupy some existing graphical annotations.

But thinking about conversion script, it is indeed a nice feature if OMEdit has a functionality renameComponent or similar. So often I do rename components and packages.

AtiyahElsheikh commented 3 years ago

Or you probably mean to automatically create Icon components for each existing component and move annotations to them? Well, that would be helpful.

sjoelund commented 3 years ago

I did not rename the Icons. There were only three Icons in version 1.0.2 and they are still employed (Except IconBase) which does not seem to be used. All other Icons have been added to occupy some existing graphical annotations.

But thinking about conversion script, it is indeed a nice feature if OMEdit has a functionality renameComponent or similar. So often I do rename components and packages.

BioChem.Icons.Substances.Package was moved to BioChem.Icons.Substances.PackageIcon. Technically if someone used the old name, that's a problem. Right?

AtiyahElsheikh commented 3 years ago

This Icon is the one I have self implemented. It is used only in one Package: the Substances package.

Edit: Just thinking may be it was already there (but not in version 1.0.2) but the Idea is it used only in one place in the whole library, the Substances package.

AtiyahElsheikh commented 3 years ago

I have removed unneeded older mo file. I initially used a class called Package (the same as within MSL) to implement the Icon of packages (Units and Substances). Then I found it is not a good practice with OMC ( due to similarity between a folder named Package and the file package.mo). So I renamed it to PackageIcon.mo.

Currently, the only place where the Icons.Substances.PackageIcon is used, is within Substances package.mo

Version 1.0.2:

Version 1.0.1:

sjoelund commented 3 years ago

Currently, the only place where the Icons.Substances.PackageIcon is used, is within Substances package.mo

You cannot know that. It's a public class so it could be used in a user's model (this is why conversion scripts exist)

AtiyahElsheikh commented 3 years ago

The Icon existed only in Version 1.0.3 as I have implemented it myself. Current implementation of substances are so generic and have the parameter c_0 as a start concentration value, without e..g. any string parameter specifying the name of the substance.

Ok but say theoretically, a user can make use of it for his own special Substances package as he was using BioChem version 1.0.3, should a conversion script now be placed somewhere? Though I don't see where the conversion script for MSL 3.2.3 -> 4.0.0 is placed?

Or is it just to check that this icon is not used anywhere within BioChem?

Also, to my understanding now, OMC does not execute conversion scripts specified in annotations?

sjoelund commented 3 years ago

That or you state in the user's guide that some things should not be extended from or used in the library as they are not considered for backwards compatibility. This could be Icons, User's Guide, anything marked Internal, etc.

AtiyahElsheikh commented 3 years ago

Relevant backward incompatibility notes added in documentation

casella commented 3 years ago

Then I found it is not a good practice with OMC ( due to similarity between a folder named Package and the file package.mo)

This may be an issue under Windows, whose file system is case insensitive. Better stay clear of that 😄