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

Updates to the PV all-in-one model #345

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

fachif
Copy link
Collaborator

@fachif fachif commented Jan 11, 2024

The following are a list of updates:

  1. The AddOnBlock (Irradiance to Power) was missing machine base power to transform the output power from MW to p.u.
  2. The user now has the option of using the addOn block with the PV model through a boolean parameter. Toggling the parameter enables or disables a realinput that allows the connection to the addOn. This works with all control modes in the PV model.
  3. Created a new Test model called PVPlantSolarIrradiance, that is documented.

The following are a list of updates:

1) The AddOnBlock (Irradiance to Power) was missing machine base power to transform the output power  from MW to p.u.
2) The user now has the option of using the addOn block with the PV model through a boolean parameter. Toggling the parameter enables or disables a realinput that allows the connection to the addOn. This works with all control modes in the PV model.
3) Created a new Test model called PVPlantSolarIrradiance, that is documented.
@dietmarw
Copy link
Member

dietmarw commented Jan 19, 2024

Wow this is rather confusing that you have created a fork for the purpose of the PR. It took me quite some time to find the commit in the log. For the future you should have a fork of OpenIPSL under your user, i.e., fachif/OpenIPSL and there you create branches on which you can develop your features (hence the name, feature-branches). Those branches can then be used to create PRs against OpenIPSL:master.

Renaming your forks to something else then the project it is forked from will cause massive confusions. I see you've done the same with Microgrids.

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.

See my inline comments.

@@ -3,6 +3,7 @@ package AddOnBlocks
"This package contains additional add ons that can be added to the original renewable models."
model IrradianceToPower "PV Array Power Output from Irradiance."

parameter Modelica.Units.SI.ApparentPower M_b = 100000000;
Copy link
Member

Choose a reason for hiding this comment

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

You are missing the description string. What is this parameter supposed to represent.

Comment on lines +61 to +64
annotation (Icon(coordinateSystem(preserveAspectRatio=false, extent={{-100,
-100},{120,100}})), Diagram(
coordinateSystem(preserveAspectRatio=false, extent={{-100,-100},{100,
100}})));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
annotation (Icon(coordinateSystem(preserveAspectRatio=false, extent={{-100,
-100},{120,100}})), Diagram(
coordinateSystem(preserveAspectRatio=false, extent={{-100,-100},{100,
100}})));

no need for these also the icon aspect ration is non-standard and would just disturb the appearance.

Comment on lines +36 to +37
__Dymola_NumberOfIntervals=5000,
__Dymola_Algorithm="Dassl"), Documentation(info="<html>
Copy link
Member

Choose a reason for hiding this comment

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

Do not use vendor annotations. Use interval instead and stop the algorithm since that is the default anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants