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

Renaming of functions CubicInterpolation_DP and CubicInterpolation_MFLOW #2780

Merged
merged 11 commits into from
Mar 7, 2019

Conversation

wischhusen
Copy link
Contributor

@wischhusen wischhusen commented Nov 21, 2018

close #2006

@beutlich beutlich changed the title Resolved issue 2006 - Renaming of functions CubicInterpolation_DP and… Renaming of functions CubicInterpolation_DP and CubicInterpolation_MFLOW Nov 21, 2018
@beutlich beutlich added this to the MSL_next-MAJOR-version milestone Nov 21, 2018
@beutlich beutlich added the L: Fluid.Dissipation Issue addresses Modelica.Fluid.Dissipation label Nov 21, 2018
@beutlich
Copy link
Member

beutlich commented Nov 21, 2018

Note, this only can be merged to master branch (targeting the next major release), once MSL v3.2.3 is released.

@hubertus65 hubertus65 self-assigned this Nov 21, 2018
@beutlich
Copy link
Member

@wischhusen Can you please provide a unit test (say ModelicaTest.Fluid.Dissipation.ConversionTest.'#2780') where conversion actually takes place? Thanks!

wischhusen added a commit that referenced this pull request Feb 5, 2019
wischhusen added a commit that referenced this pull request Feb 5, 2019
@wischhusen
Copy link
Contributor Author

Added the conversion scirpt to Modelica/Resources/Scripts/Dymola and a Tester (Issue2780) to ModelicaTest.Fluid.Dissipation.ConversionTest.

I'm a bit puzzled about the correct version numbers for the change. It should be 3.2.4, right?

@wischhusen wischhusen closed this Feb 5, 2019
@wischhusen wischhusen reopened this Feb 5, 2019
@dietmarw
Copy link
Member

dietmarw commented Feb 5, 2019

The next version is 4.0.0

@wischhusen
Copy link
Contributor Author

Ok, so the Dymola conversion script should be named
ConvertModelica_from_3.2.3_to_4.0.0.mos ?

@dietmarw
Copy link
Member

dietmarw commented Feb 5, 2019

Exactly. 👍

wischhusen added a commit that referenced this pull request Feb 5, 2019
@wischhusen
Copy link
Contributor Author

Done

@dietmarw
Copy link
Member

dietmarw commented Feb 5, 2019

Please do not push branches to the MSL repository but use your own fork for that. Also the last correction is not present in the PR here. So you might have pushed to the wrong place.

@wischhusen
Copy link
Contributor Author

There should be a branch issue2780. Anyhow, if that cannot be merged I will proceed with the fork.

@dietmarw
Copy link
Member

dietmarw commented Feb 5, 2019

Yes best to make a new PR based on a branch from your fork and also make sure that there is a test model in ModelicaTest that would trigger that conversion.

wischhusen added a commit to wischhusen/Modelica-1 that referenced this pull request Feb 5, 2019
@wischhusen wischhusen closed this Feb 5, 2019
@wischhusen wischhusen reopened this Feb 5, 2019
Copy link
Member

@dietmarw dietmarw left a comment

Choose a reason for hiding this comment

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

I was wondering if the location Scripts/Dymola is still the appropriate one since the conversion script is standardised for MLS 3.4 so in MSL 4.0.0 the standard script should probably be placed directly under Scripts?
@beutlich ?

@beutlich
Copy link
Member

beutlich commented Feb 6, 2019

I was wondering if the location Scripts/Dymola is still the appropriate one since the conversion script is standardised for MLS 3.4 so in MSL 4.0.0 the standard script should probably be placed directly under Scripts?

See #2819 for the corresponding PR.

@beutlich
Copy link
Member

beutlich commented Feb 6, 2019

Sorry for using this PR as guinea pig for the conversion. We will care for the proper location of the conversion script and the conversion tester.

beutlich pushed a commit that referenced this pull request Feb 6, 2019
beutlich pushed a commit that referenced this pull request Feb 6, 2019
beutlich
beutlich previously approved these changes Feb 6, 2019
Modelica/package.mo Outdated Show resolved Hide resolved
beutlich
beutlich previously approved these changes Feb 7, 2019
@beutlich
Copy link
Member

beutlich commented Feb 8, 2019

This is an important PR since it adds ConvertModelica_from_3.2.3_to_4.0.0.mos and ModelicaTestConversion4.mo, which all other issues requiring conversion depend on.

@hubertus65 Can you please run the merge.

@beutlich
Copy link
Member

@hubertus65 Can you please run the merge.

Friendly reminder.

@hubertus65
Copy link
Member

I have been traveling quite a bit. I'll try to find some block time for this. Any other tasks for me I missed?

@beutlich
Copy link
Member

beutlich commented Mar 3, 2019

Any other tasks for me I missed?

Yes, also #2816 and #2818 await your review.

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.

Looks good.
I noticed one minor issue - but it isn't really part of this PR so I believe that should be a separate PR.

The minor issue is that conversion from MSL 2 could specify to="3.2.3"; so that the libraries using MSL 2 could be converted to MSL 4. (Which is super-rare and those conversions weren't changed anyway.)

@dietmarw
Copy link
Member

dietmarw commented Mar 4, 2019

The minor issue is that conversion from MSL 2 could specify to="3.2.3"; so that the libraries using MSL 2 could be converted to MSL 4. (Which is super-rare and those conversions weren't changed anyway.)
I discussed this with @beutlich and decided against including MSL 2 conversions since as far as I know, most of the current tools don't even support MSL 2 anymore. So no point.

@hubertus65 hubertus65 merged commit 4da19a4 into master Mar 7, 2019
@beutlich beutlich deleted the issue2006 branch March 7, 2019 15:21
@beutlich beutlich added the task General work that is not related to a bug or feature label Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Fluid.Dissipation Issue addresses Modelica.Fluid.Dissipation 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.

Names of CubicInterpolation_DP() und CubicInterpolation_MFLOW() are misleading
5 participants