Skip to content

Comments

Physical units#23

Merged
AtiyahElsheikh merged 25 commits intoOpenModelica:developfrom
AtiyahElsheikh:PhysicalUnits
Apr 17, 2021
Merged

Physical units#23
AtiyahElsheikh merged 25 commits intoOpenModelica:developfrom
AtiyahElsheikh:PhysicalUnits

Conversation

@AtiyahElsheikh
Copy link
Collaborator

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

AtiyahElsheikh and others added 23 commits July 1, 2020 13:55
Usage of MSL 4.0.0 after applying the conversion script
The Modelica grammar does not allow annotations except at the end of a class.
* Convert to MSL 4.0.0

* Update version number to 1.0.3
# Conflicts:
#	BioChem/Interfaces/Reactions/Basics/FourSubstrates.mo
#	BioChem/Units/package.mo
@AtiyahElsheikh
Copy link
Collaborator Author

Only the last two commits to review

@AtiyahElsheikh AtiyahElsheikh requested a review from sjoelund April 7, 2021 14:11
@sjoelund
Copy link
Member

sjoelund commented Apr 7, 2021

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@sjoelund
Copy link
Member

sjoelund commented Apr 8, 2021

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
Copy link
Collaborator Author

AtiyahElsheikh commented Apr 8, 2021

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
Copy link
Collaborator Author

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
Copy link
Member

sjoelund commented Apr 9, 2021

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
Copy link
Collaborator Author

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
Copy link
Member

sjoelund commented Apr 9, 2021

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
Copy link
Collaborator Author

Relevant backward incompatibility notes added in documentation

@AtiyahElsheikh AtiyahElsheikh linked an issue Apr 9, 2021 that may be closed by this pull request
@casella
Copy link

casella commented Apr 9, 2021

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 😄

@AtiyahElsheikh AtiyahElsheikh merged commit ca3c6a8 into OpenModelica:develop Apr 17, 2021
@AtiyahElsheikh AtiyahElsheikh deleted the PhysicalUnits branch April 17, 2021 07:35
AtiyahElsheikh added a commit to AtiyahElsheikh/BioChem that referenced this pull request Apr 17, 2021
@AtiyahElsheikh AtiyahElsheikh linked an issue Apr 17, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Annotations associated with physical units Icons

3 participants