Skip to content
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

Reorganise SIunits into Units package #3379

Merged
merged 18 commits into from
Feb 1, 2020

Conversation

dietmarw
Copy link
Member

This fixes #712 by reorganising the SIunits package into a Units package

I checked the following libraries (Dymola 2020, non-pedantic mode, see #3378 why) :

  • Complex.mo
  • Modelica
  • ModelicaReference
  • ModelicaServices
  • ModelicaTest
  • ModelicaTestConversion4.mo
  • ModelicaTestOverdetermined.mo
  • ObsoleteModelica4.mo

@dietmarw dietmarw added L: SIunits Issue addresses Modelica.SIunits task General work that is not related to a bug or feature labels Jan 30, 2020
@dietmarw dietmarw added this to the MSL4.0.0 milestone Jan 30, 2020
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall quality of the PR is very good. However there are two issues we need to discuss, being a design issue and a naming issue.

  1. Naming: Modelica.Units.Other looks odd or incomplete when you have something like import Modelica.Units.Other; Other.Time_day v;. Since there still is import NonSI = Modelica.Units.Other; in the MSL, why do not we rename Other to NonSI?

  2. Design: b2b8ae2 imports Modelica.Units.SI everywhere. This kind of global namespace pollution is considered bad practice in software engineering and I strongly prefer to avoid it here. A compromise would be to add this import statement for every package where it actually is required. For example, it is not needed for Modelica.UsersGuide, thus there is no need to import it there.

@beutlich
Copy link
Member

@christiankral @HansOlsson @MartinOtter It would be very hepful if we can discuss my concerns tomorrow (31st of Jan.) during day-time. Thanks a lot.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments:

  1. If we want to change this - it is clear that this is the time.
  2. I agree with the concern regarding Other.
  3. I agree that a top-level import used everywhere is a bit odd style, and often I find that the full name is often good enough. However, I don't have strong feelings about this.
  4. I noticed that we internally recognize "Modelica.SIunits" and will need to update that - I assume it might be similar for other tool vendors; but I don't see this as a blocking concern. (E.g., Ctrl-Shift-U inserts "Modelica.SIUnits." and starts text-completion.)

@beutlich
Copy link
Member

One compromise would be to remove the top-level import from package Modelica and add it as second-level import where required. This way, we can avoid the import for Modelica.UsersGuide, Modelica.Icons and Modelica.Units itself.

@tobolar
Copy link
Contributor

tobolar commented Jan 31, 2020

@beutlich

why do not we rename Other to NonSI?

Strictly following BIPM, there is defined a limited number of non-SI units accepted for use with the SI Units. Units.Others or formerly SIunits.Conversions.NonSIunits include more units then specified.
I like NonSI if we consider/document it uncoupled to BIPM.

See #712 (comment).

@casella
Copy link
Contributor

casella commented Jan 31, 2020

The overall quality of the PR is very good. However there are two issues we need to discuss, being a design issue and a naming issue.

1. Naming: Modelica.Units._Other_ looks odd or incomplete when you have something like `import Modelica.Units.Other; Other.Time_day v;`. Since there still is `import NonSI = Modelica.Units.Other;` in the MSL, why do not we rename Other to NonSI?

I fully agree with this. NonSI is much more informative than Other

2. Design: [b2b8ae2](https://github.com/modelica/ModelicaStandardLibrary/commit/b2b8ae23a775d1ab7612b274c02d5d119cd50b9a) imports Modelica.Units.SI everywhere. This kind of global namespace pollution is considered bad practice in software engineering and I strongly prefer to avoid it here. A compromise would be to add this import statement for every package where it actually is required. For example, it is not needed for Modelica.UsersGuide, thus there is no need to import it there.

I may agree in principle here. However, I don't see much of a problem in having SI defined in the very few second-level packages that don't need it. I guess nobody will use those two letters to indicate anything else than the metric unit system. Well, SI means yes in Italian, so one may want to define a macro for yes/no replies when developing a GUI in Italian, but Modelica is not C and is not used to write GUIs, so I don't see this as a big deal.

On the other hand, a global import statement makes it very clear that we are using the same import symbol throughout the Modelica standard library, and also that we are implicitly recommending other library developers to do so. After all, many libraries have an SI import statement, which I guess is a pattern borrowed from the MSL.

So I'm slightly in favour of keeping it like this, but I have no strong opinion on this topic.

@beutlich
Copy link
Member

beutlich commented Jan 31, 2020

Proposals for tonight and Alpha.1.

  1. Rename Other to NonSI.
  2. Avoid top-level imports and add second-level imports (in package Modelica only).

What do you think?

TODO:
 - [X] add conversion script entries
 - [ ] update all references in MSL
 - [ ] update all references and simplify import statements in
   - [ ] Complex
   - [ ] ObsoleteModelica4
   - [ ] ModelicaTestOverdetermined
 - [ ] update the documentation of the package
Copy link
Contributor

@christiankral christiankral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, looks like a sound solution now

@beutlich beutlich merged commit cf26871 into modelica:master Feb 1, 2020
beutlich added a commit that referenced this pull request Feb 1, 2020
@dietmarw dietmarw deleted the t712-SIunitsRename branch February 13, 2020 10:23
@beutlich beutlich added L: Units Issue addresses Modelica.Units and removed L: SIunits Issue addresses Modelica.SIunits labels Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Units Issue addresses Modelica.Units task General work that is not related to a bug or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise/revisit all SI-units listed in Modelica.SIunits
6 participants