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
Clean-up and reimplementation of PowerFactory solar models #266
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not yet looked at the models themselves but noticed that the hierarchy is off. PowerFactory is a product of the company named DigSILENT. Now why would you have a subfolder DigSILENT inside PowerFactory? That is the wrong way round. Anyway it would be sufficient to only name the software and not the company. We also don't do Siemens/PSSE
. So please fix the structure.
OK I see. In that case it does make sense. So basically DIgSILENT have still not come to terms with their own rebranding of their product. |
Where now stands DigSILENT, there was once KTH. :) |
Only it should be DIgSILENT |
@@ -0,0 +1,114 @@ | |||
within OpenIPSL.Electrical.Solar.PowerFactory.DIgSILENT.Auxiliary; | |||
|
|||
model ActivePowerController |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a short descriptive string to this and also to all the other models which are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fa63456.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things need fixing:
- Still some typos of DIgSILENT present
- Use
Types.*
instead ofSI.*
and the latter only if not present underTypes.*
- remove local imports of SI and C. Those imports are already present on the top-level and don't need to get repeated.
- input/output connectors need to be arranged on the grid as such that they don't cause steps when connecting (i.e., multiple of 40 on the icon layer). Also check the MSL conventions for good style icon layout:
Modelica.UsersGuide.Conventions.Icons
- align blocks and connections on the grid, do not use streched icons (See
Solar.PowerFactory.DIgSILENT.PV_Plant
for a bad example) - avoid counters as part of instance names if only one instance is present
By this you mean all the blocks from MSL? Otherwise I can't see where they happened. |
One example would be |
@dietmarw Hopefully all of the things you've listed have now been fixed. |
Placement(visible = true, transformation(origin = {70, 30}, extent = {{10, -10}, {-10, 10}}, rotation = 0))); | ||
inner OpenIPSL.Electrical.SystemBase SysData annotation( | ||
Placement(visible = true, transformation(origin = {-70.5, 90}, extent = {{-29.5, -10}, {29.5, 10}}, rotation = 0))); | ||
OpenIPSL.Electrical.Solar.PowerFactory.DIgSILENT.PV_Plant pv_plant(M_b(displayUnit = "V.A") = 0.5e6, P_0 = 300000, angle_0 = 0, v_0 = 1) annotation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it V.A or VA here after displayUnit? @tinrabuzin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcelofcastro That's what OpenModelica picks up from MSL. It was not me. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool cool, just wanted to make sure it was not a typo haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look like it so I went and checked. The unit abbreviation does not make sense if you ask me but maybe there's something that I don't know...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the dot is just to make the multiplication of units explicit. It's a bit weird to me as well but @dietmarw will say that I may have a problem with units in Modelica hahaha Anyway, good that's not a typo!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be VA
. I guess some version of OMEdit did screw this up. I changed it now to a more meaningful MVA
<td><p>This model has not been verified against PowerFactory.</p></td> | ||
</tr> | ||
<tr> | ||
<td><p>Description</p></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's no description, you can remove this row and this is valid for every model you have here in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Done in 7d764f2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
</p> | ||
</html>", revisions = "<html> | ||
<table cellspacing=\"1\" cellpadding=\"1\" border=\"1\"><tr> | ||
<td><p>Model Name</p></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need this Model Name row as well. But I will leave it to @dietmarw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcelofcastro @tinrabuzin I guess the model name here is meant as reference back to what it is called in PowerFactory. So maybe still of use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, so we can use it for sure.
connect(voltage_source.f0, frequency.y) annotation( | ||
Line(points = {{40, 4}, {50, 4}, {50, 30}, {60, 30}}, color = {0, 0, 127})); | ||
protected | ||
annotation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to include here what the user should see when executing this example. For instance, take a look at Examples.Controls.PSSE.TG.GGOV1, there we started implementing this idea of listing what the user should see when simulating the model. It can be super brief but it helps to have an idea of what is being simulated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Check out 6f41a4f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's pretty nice now!
78de773
to
303ab10
Compare
Given the delay of 2.0.0 I would not be against including this and #267 in the final 2.0.0 release. @marcelofcastro @lvanfretti, any thoughts? |
I agree, @dietmarw. Let's include these changes in 2.0.0. |
Co-authored-by: Marcelo de C. Fernandes <decasm3@rpi.edu>
Co-authored-by: Marcelo de C. Fernandes <decasm3@rpi.edu>
Still contains the original code that
b334ed1
to
59ba050
Compare
I needed to change the |
Not all functionality is implemented and not all models are checked yet, but at least the models are usable now.