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

Refs #3593: Fix documentation of quasi-static machine examples #3620

Merged
merged 2 commits into from
May 26, 2021

Conversation

christiankral
Copy link
Contributor

I checked the quasi static induction machine examples and updated the documentation to match the actual variables.

@christiankral
Copy link
Contributor Author

christiankral commented Aug 24, 2020

The following models use the instance name iSensorQS instead of currentQuasiRMSSensorQS which makes the example collection a little inconsistent

  • Modelica.Magnetic.QuasiStatic.FundamentalWave.Examples.BasicMachines.InductionMachines.IMC_Characteristics
  • Modelica.Magnetic.QuasiStatic.FundamentalWave.Examples.BasicMachines.InductionMachines.IMS_Characteristics

@tobolar @beutlich Are we allowed to rename a sensor in the master branch? If yes, I would unify the appearance of the sensor names.

@christiankral christiankral added the L: Magnetic.QuasiStatic Issue addresses Modelica.Magnetic.QuasiStatic label Aug 24, 2020
@christiankral christiankral changed the title Refs #3593: Fix documentation of quasi static induction machine examples Refs #3593: Fix documentation of quasi static machine examples Aug 24, 2020
@christiankral
Copy link
Contributor Author

There is one more issue with a synchronous machine example which I am going to fix.

@tobolar
Copy link
Contributor

tobolar commented Sep 7, 2020

Are we allowed to rename a sensor in the master branch? If yes, I would unify the appearance of the sensor names.

Do you mean to rename a sensor element in that particular example model?

@christiankral
Copy link
Contributor Author

Are we allowed to rename a sensor in the master branch? If yes, I would unify the appearance of the sensor names.

Do you mean to rename a sensor element in that particular example model?

Yes, this is what I mean

@tobolar
Copy link
Contributor

tobolar commented Sep 8, 2020

IMO this is no problem when only an example is concerned. I remember there has been some comment on it by (I guess) @dietmarw but I can't find it anymore.

@dietmarw
Copy link
Member

dietmarw commented Sep 8, 2020

First of all you need to determine what is meant by "master branch". The question is what next version the master branch is aiming at. Backward compatible, i.e., 4.1.0, or non-backward compatible, i.e., 5.0.0. As far as I know this has not yet been decided within MAP-LIB. In the latter case there is no restrictions on renames as long as conversion scripts are given. In the former case we tended to allow renames within examples even if they break backward compatibility since the argument is that users should not let their usermodels extend from examples (copying is fine but not extend).

@beutlich beutlich changed the title Refs #3593: Fix documentation of quasi static machine examples Refs #3593: Fix documentation of quasi-static machine examples Jan 19, 2021
@beutlich beutlich requested a review from tobolar January 19, 2021 22:01
@christiankral christiankral linked an issue Mar 15, 2021 that may be closed by this pull request
@christiankral
Copy link
Contributor Author

OK, I propose to separate the issues which became mixed up in this PR.

  • Fix documentation: this is covered by this PR
  • Change spelling (currently, e.g., smpm|smpmQS.tauElectrical) of multiple similar variables as suggested by @tobolar: I propose to create a different ticket and PR, respectively, as this issue affects some more models of the MSL
  • Unify naming of sensor: Optionally create a new PR as this issue is not directly related with the documentation; it may also require some more discussion if we are fine with renaming instance names of MSL examples in the future

So if there are not objections with this approach, I am asking @tobolar and @dietmarw to re-evaluate this PR so that we can proceed.

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.

Looks good to me. I don't think parentheses would be strictly necessary in case of the choices but would not harm either. I leave that to the LOs to decide.

@dietmarw
Copy link
Member

I've rebased the PR on current master in order to get the CI checks working.

@AHaumer AHaumer merged commit 0dc38e4 into modelica:master May 26, 2021
@beutlich beutlich removed the request for review from tobolar May 26, 2021 18:15
@beutlich beutlich added this to the MSL4.1.0 milestone May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Magnetic.QuasiStatic Issue addresses Modelica.Magnetic.QuasiStatic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check documentation of induction machines' examples
5 participants