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

Initialization of Polyphase.Basic.{Capacitor, Inductor} #4488

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

AHaumer
Copy link
Contributor

@AHaumer AHaumer commented Oct 22, 2024

This is to test the support of tools for proper initialization menu of arrays, as discussed in #4457 .
See Modelica.Electrical.PolyPhase.Examples.TransformerYY
If this works well, the same style of initialization will be implemented for other polyphase models and machines.

@AHaumer AHaumer added enhancement New feature or enhancement L: Electrical.Polyphase Issue addresses Modelica.Electrical.Polyhase labels Oct 22, 2024
@AHaumer AHaumer added this to the MSL4.2.0 milestone Oct 22, 2024
Copy link
Contributor

@maltelenz maltelenz left a comment

Choose a reason for hiding this comment

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

In the example, it looks fine in System Modeler, same as screenshots in #4457 (comment)

However, if I drag out a new Inductor, I don't get to edit i easily in System Modeler. I guess the heuristics of when to show variable start attributes when there are no annotations can be discussed, but should we add showStartAttribute explicitly in TwoPlug, to make the intention very clear from the library?

SI.Current i[m] "Currents flowing into positive polyphase plugs" annotation(Dialog(showStartAttribute=true));

@HansOlsson
Copy link
Contributor

In the example, it looks fine in System Modeler, same as screenshots in #4457 (comment)

However, if I drag out a new Inductor, I don't get to edit i easily in System Modeler. I guess the heuristics of when to show variable start attributes when there are no annotations can be discussed, but should we add showStartAttribute explicitly in TwoPlug, to make the intention very clear from the library?

SI.Current i[m] "Currents flowing into positive polyphase plugs" annotation(Dialog(showStartAttribute=true));

Good idea in some form, but I don't think that should be in the base-class TwoPlug.

@christiankral
Copy link
Contributor

I agree, as we sometimes have initial currents (in coils) and voltages (in capacitors), depending on the component state variables.

@maltelenz
Copy link
Contributor

I guess another choice is to "lift" i with an alias to the Inductor model and add the showStartAttribute annotation on that alias?

@AHaumer
Copy link
Contributor Author

AHaumer commented Oct 23, 2024

@maltelenz that's what we wanted to avoid: an additional alias variable.
But ok I'll change the PR (it seems to make it easier for tools)
and let's see whether we get an agreement on that (@christiankral ?).

@AHaumer AHaumer requested a review from maltelenz October 23, 2024 17:12
Copy link
Contributor

@maltelenz maltelenz left a comment

Choose a reason for hiding this comment

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

It now shows up nicely in System Modeler.

I agree it's a shame that an alias variable is needed, but I see no other deterministic way to make this work cross-tool.

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 to me.
I would prefer the each, but either works.

Copy link
Contributor

@casella casella left a comment

Choose a reason for hiding this comment

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

LGTM

@casella
Copy link
Contributor

casella commented Oct 24, 2024

I would prefer the each, but either works.

As I wrote in the comment, writing each seems to somewhat imply that normally those attributes are the same for all the elements of the array. This is really not the case.

@casella casella merged commit bf60ef8 into modelica:master Oct 24, 2024
11 checks passed
@AHaumer AHaumer deleted the PolyPhaseInitialization branch October 24, 2024 18:30
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: Electrical.Polyphase Issue addresses Modelica.Electrical.Polyhase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants