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

FluxTubes: some improvements in shapes and material #3993

Merged
merged 20 commits into from
Jun 20, 2022

Conversation

AHaumer
Copy link
Contributor

@AHaumer AHaumer commented Jun 11, 2022

Checked all shapes, FluxTubes and QuasiStatic.FluxTubes in parallel

@AHaumer AHaumer added bug Critical/severe issue L: Magnetic.FluxTubes Issue addresses Modelica.Magnetic.FluxTubes L: Magnetic.QuasiStatic Issue addresses Modelica.Magnetic.QuasiStatic labels Jun 11, 2022
@AHaumer AHaumer self-assigned this Jun 11, 2022
@AHaumer AHaumer enabled auto-merge June 11, 2022 18:59
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.

The new images have no longer a transparent background. Was that on purpose? This will look strange when compared to the other images that are still transparent.

@beutlich beutlich removed their request for review June 13, 2022 08:21
@beutlich beutlich disabled auto-merge June 13, 2022 08:22
@beutlich beutlich enabled auto-merge (squash) June 13, 2022 08:23
This reverts commit ea740a58823b9ce65f6b54d94a1208dfe7159f98.
@dietmarw
Copy link
Member

@AHaumer I also had to revert your "???" commit once again here. Please follow the instruction I gave you of basing your future PRs on the current master (rather than keeping to merge the current master into your "broken" local master with that "???" commit).

@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 13, 2022

The new images have no longer a transparent background. Was that on purpose? This will look strange when compared to the other images that are still transparent.

Sorry this was by mistake, I'll correct that. I suppose you mean the images for HollowCylinder RadialFlux and Axial Flux?

@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 13, 2022

@AHaumer I also had to revert your "???" commit once again here. Please follow the instruction I gave you of basing your future PRs on the current master (rather than keeping to merge the current master into your "broken" local master with that "???" commit).

Sorry I didn't get that. Where did you give these instructions (PM or in a PR)? I don't know how to fix my broken master (and I have no idea how it got broken). Thanks in advance @dietmarw !

@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 13, 2022

The new images have no longer a transparent background. Was that on purpose? This will look strange when compared to the other images that are still transparent.

Sorry this was by mistake, I'll correct that. I suppose you mean the images for HollowCylinder RadialFlux and Axial Flux?

I'm a little bit confused ... to me all images have a white background, also the older ones?
BTW, I don't see the difference between white and transparent background (at least in Dymola, in the parameter menu).

1 similar comment
@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 13, 2022

The new images have no longer a transparent background. Was that on purpose? This will look strange when compared to the other images that are still transparent.

Sorry this was by mistake, I'll correct that. I suppose you mean the images for HollowCylinder RadialFlux and Axial Flux?

I'm a little bit confused ... to me all images have a white background, also the older ones?
BTW, I don't see the difference between white and transparent background (at least in Dymola, in the parameter menu).

@dietmarw
Copy link
Member

dietmarw commented Jun 14, 2022

I'm a little bit confused ... to me all images have a white background, also the older ones? BTW, I don't see the difference between white and transparent background (at least in Dymola, in the parameter menu).

@AHaumer Have a look at the changed files here and scroll right to the bottom where GitHub shows you the changes to the images. As you can see the original had a checkered background (symbolising the transparency) the new ones have a plain white background which is not transparent. Dymola would only show you this if you started it in dark mode (-dark or /dark)
image

before it looked like this:
image

changed background of shapes images to transparent
@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 14, 2022

@dietmarw sorry I didn't see that. Also two other images have white instead of transparent background - corrected all four.
BTW please point out once again for me how I can repair my "broken" master. Thanks in advance!

@dietmarw
Copy link
Member

dietmarw commented Jun 14, 2022

@AHaumer Simply checkout your local master and reset it to upstream master and then you can create your topic branches for future PRs based on that again.

git fetch --all
git checkout master
git reset --hard upstream/master  # if your remote for MSL is called upstream

(edited to also fetch the latest states of remote first)

@beutlich
Copy link
Member

Looks good to me. @AHaumer maybe you could also add the source files of the images next to the pngs?

You still can add them to this repo and exclude them from packaging in the attributes files like done here:

Modelica/Resources/Documentation/Clocked/Modelica_Synchronous.pptx export-ignore
Modelica/Resources/Documentation/Fluid/Stream-Connectors-Overview-Rationale.ppt export-ignore
Modelica/Resources/Documentation/Mechanics/Figure_PlanarLoopAnalytic.ppt export-ignore

See also #2603.

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.

Some of the materials are brand names of the manufacturer Vacuumschmelze. The manufacturer uses the UPPERCASE spelling but in Modelica we have all kinds of different spelling mixed: For the sake of simplicity I propose to keep the used record names in order to avoid changing the record name in the next major release by means of a conversion script. In this case we should at least use the manufacturer name in the label or at least unify the spelling of the labels.

In the following PNG images the angle alfa shall be changed to either alpha or the greek letter $\alpha$:

  • Modelica/Resources/Images/Magnetic/FluxTubes/Shapes/Circumferential.png
  • Modelica/Resources/Images/Magnetic/FluxTubes/Shapes/Toroid.png

@christiankral
Copy link
Contributor

As this ticket includes a bug fix, too, it shall be back-ported to MSL 4.0.0.

@dietmarw
Copy link
Member

If that bugfix part is really important it should be isolated and not also mixed with all the extensions that this PR now contains. So question is, is it really such a severe bug (which one actually? Ticket?) or can we live with that until the next minor release.

AHaumer and others added 6 commits June 15, 2022 18:18
…ermenorm3601K3.mo

Co-authored-by: Christian Kral <dr.christian.kral@gmail.com>
…oferS2.mo

Co-authored-by: Christian Kral <dr.christian.kral@gmail.com>
…9SMn28K.mo

Co-authored-by: Christian Kral <dr.christian.kral@gmail.com>
…acoflux50.mo

Co-authored-by: Christian Kral <dr.christian.kral@gmail.com>
@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 15, 2022

If that bugfix part is really important it should be isolated and not also mixed with all the extensions that this PR now contains. So question is, is it really such a severe bug (which one actually? Ticket?) or can we live with that until the next minor release.

I just double-checked in detail:
I've been mistaken by some too complicated formulation of formulas for cross section (area A) and magnetic conductance Gm.
The most annoying topic was that some shapes in QuasiStatic.FluxTubes already had been improved with central angle alpha whereas the corresponding shapes in (transient) FluxTubes had fixed central angle = 2*pi.
Therefore no need for backporting but in my humble opinion improvements.

I'll replace "bug" by "improvement" in the title and the label of this PR.

@AHaumer AHaumer changed the title FluxTubes: fix some bugs in shapes FluxTubes: some imporvements in shapes and material Jun 15, 2022
@AHaumer AHaumer changed the title FluxTubes: some imporvements in shapes and material FluxTubes: some improvements in shapes and material Jun 15, 2022
@AHaumer AHaumer added enhancement New feature or enhancement and removed bug Critical/severe issue labels Jun 15, 2022
@AHaumer
Copy link
Contributor Author

AHaumer commented Jun 15, 2022

Looks good to me. @AHaumer maybe you could also add the source files of the images next to the pngs?

You still can add them to this repo and exclude them from packaging in the attributes files like done here:

Modelica/Resources/Documentation/Clocked/Modelica_Synchronous.pptx export-ignore
Modelica/Resources/Documentation/Fluid/Stream-Connectors-Overview-Rationale.ppt export-ignore
Modelica/Resources/Documentation/Mechanics/Figure_PlanarLoopAnalytic.ppt export-ignore

See also #2603.

I've included the sources (Visio) for images. Thanks @beutlich for the hint about gitattributes.

@christiankral
Copy link
Contributor

In the following PNG images the angle alfa shall be changed to either alpha or the greek letter :

  • Modelica/Resources/Images/Magnetic/FluxTubes/Shapes/Circumferential.png
  • Modelica/Resources/Images/Magnetic/FluxTubes/Shapes/Toroid.png

This is not yet resolved.

@christiankral christiankral self-requested a review June 20, 2022 09:37
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 now

@beutlich beutlich merged commit e3d183e into modelica:master Jun 20, 2022
@beutlich beutlich removed the request for review from ThomasBoedrich June 20, 2022 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement L: Magnetic.FluxTubes Issue addresses Modelica.Magnetic.FluxTubes L: Magnetic.QuasiStatic Issue addresses Modelica.Magnetic.QuasiStatic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants