-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Change setter for losses_model to allow it to set multiple loss funct… #1084
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
Conversation
…ions, anticipating more complex loss models. Adds self._dc_losses
The strategy I used here is to use the setter on It might be more appropriate to have a separate setter for each loss function, instead. But I thought it makes sense to keep a single user parameter that gets passed when setting up a Would appreciate any comments on this direction before continuing further with possible future PRs to add on other loss functions, as discussed in #1069. |
…hms based on a user specifed loss percent at stc.
…function for calculating dc losses from a loss percentage at stc based on the method employed by pvsyst.
I added in some trial functions to |
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'm OK with the direction this PR is taking. ModelChain.losses_model
becomes an identifier for a set of loss models. Although here only a DC loss model is added, a set can grow to include irradiance, shading, soiling, etc. The set approach seems to be a practical strategy since loss models can overlap or conflict in different implementations (e.g., shading can be modeled as a reduction in irradiance, or could be accounted for by a factor applied to DC power).
pvlib/pvsystem.py
Outdated
Calculates the equivalent resistance of the wires from a percent | ||
ohmic loss at STC, defined by the user as an input to loss_parameters. | ||
Equivalent resistance is calculated with the fucntion: |
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.
Calculates the equivalent resistance of the wires from a percent | |
ohmic loss at STC, defined by the user as an input to loss_parameters. | |
Equivalent resistance is calculated with the fucntion: | |
Calculates the equivalent resistance of the wires from a percent | |
ohmic loss at STC. | |
Equivalent resistance is calculated with the fucntion: |
pvlib/modelchain.py
Outdated
@@ -955,6 +960,19 @@ def no_extra_losses(self): | |||
self.losses = 1 | |||
return self | |||
|
|||
def pvsyst_dc_losses(self): |
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.
This is the only DC loss applied by pvsyst? If no, is the plan to add to this method when other DC loss models are implemented?
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.
In PVsyst you can also input ohms directly, which we could support pretty easily I think with a different loss parameter that uses the same model chain function but bypasses the equivalent resistance function. I want to investigate that more to confirm the method and then will work to implement.
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'm generally good with the basic function and method. I'm concerned about how this works with ModelChain
. I think we're going to need to start breaking up the losses_model
into a handful of options. From the pvwatts_losses
kwargs, snow, soiling, wiring, and shading could all be treated with their own ModelChain
kwargs.
pvlib/pvsystem.py
Outdated
modules_per_string: numeric, defualt 1 | ||
strings_per_inverter: numeric, defualt 1 |
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.
spelling
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.
@ncroft-b4 lmk if you'd like some help writing tests
@cwhanse I made an attempt at the tests. I would appreciate a review that I met the intention there. |
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.
@ncroft-b4 tests look OK so far. For coverage of line L953, the aoi method test is a model.
Would like to close this out in favor of #1168 which includes compatibility for arrays in the latest updates. |
…ions, anticipating more complex loss models. Adds self._dc_losses
docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).