Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DC Ohmic Losses for pvsystem and modelchain #1168
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
DC Ohmic Losses for pvsystem and modelchain #1168
Changes from 5 commits
5c79590
b6fa6f6
8b6769c
18228de
ab5ecff
f63a24b
0db5afe
a8a911e
3c7b85d
8749450
4500bc7
547f6c1
68857ab
26be1fe
f96ac03
ba92da6
19728cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
any idea why codecov says this line isn't tested? It looks like it should be.
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.
That was confusing for me as well. And now it is telling me that blank lines 1082 and 1095 are not covered, and that 1090 is not covered which it should be as there is a test specifically for
dc_ohmic_model='no_loss'
. I'd welcome an assist from someone more familiar with the tests on what might be leading to the failed coverage if anyone has an idea.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 ran a coverage report locally and cannot reproduce the codecov results. Adding exceptions to these lines causes errors, as you would expect.
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 just realized this might need to be rewritten to handle the case where there are multiple arrays and so
dc_ohms_from_percent
returns a tuple instead of a single value. In that case what willself.dc[i_mp]
be? Commenting to mark to investigate and fix.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.
self.results.dc
will be a tuple ofDataFrame
.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.
Thanks. I added functionality for multiple arrays in the latest commit. I think all of the comments are addressed now and it is ready for the review and push forward.
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.
sorry, should have caught this earlier.... it seems repetitive to call this parameter
array_losses_parameters
since it's already on anArray
. I suppose there is some risk of user confusion if this and thePVSystem
losses_parameters
are called the same thing, but it doesn't hurt to have extra keys in these dictionaries. It's unlikely that would change in the future.Maybe we should live with the current API on master in an alpha release or two and then maybe take up renaming in a follow up issue. I could see something like #1176 providing more clarity on this.
Thoughts @ncroft-b4 @cwhanse ?
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 either
array_losses_parameters
orArray.losses_parameters
(andPVSystem.losses_parameters
) in this PR.I think as loss models are added to pvlib and we work those models into ModelChain, we'll end up refactoring the parameter containers.
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 I think my concern was making it distinguishable from the
PVSystem
parameter. Also, to keep compatibility with initializing aPVSystem
directly, there needs to be a difference between thelosses_parameters
that would be passed into theArray
and the ones that would stay with thePVSystem
.That makes me realize that I added
array_losses_parameters
to the list of keys that get passed toArray
fromPVSystem
if anArray
is not specified, but I did not addarray_losses_parameters
explicitly to the documentation forPVSystem
or to theinit
. That makes sense to be becausearray_losses_parameters
is only anArray
parameter, but it does make it different from the otherArray
parameters that are still part ofPVSystem
by legacy. I'm good with it as it is but if you want to keep it uniform with the legacy parameters we could change it.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 wonder if there is a way to extend this to the other DC module models. If
module_parameters
is taken from the CEC database, it will includeV_mp_ref
andI_mp_ref
.The other cases are Pvsyst, SAPM and PVWatts.
For Pvsyst, pvlib provides no parameter database. The pvsyst DC power functions don't require V_mp_ref or I_mp_ref, so the user would need to know that these must be added.
For SAPM, the expected names are
Vmpo
andImpo
.For PVWatts, the ohmic losses model can't be applied since PVWatts only deals with power (not voltage and current separately).
V_mp_ref
,I_mp_ref
anddc_ohmic_percent
are required by the downstream method. Maybe raise a ValueError here if any are missing at this step.In the ModelChain
dc_ohms_from_percent
method, we could checkdc_model
and raise a more informative message how to correct the ValueError.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. I need to sort out the relations between the DC models and the module parameters, which have always been a bit confusing, I admit. I would like to make this applicable for CEC, SAPM, and PVsyst models. Sounds like for SAPM, I can add the option to use Vmpo and Impo if those are present in the module parameters. But for PVsyst, it may require a different workflow, or like you say, to raise a ValueError to say that those parameters need to be added. I will look into this to add an update.
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.
Do you know if the Pvsyst software names these two quantities? If so, I'd use those names. A user will have to add them to
module_parameters
regardless since there's no data source from which to inherit the names.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 added a section that checks from and adds the parameters in sets of either
V_mp_ref
,I_mp_ref
;Vmpo
,Impo
; orVmpp
, Impp` (pvsyst-like, from the documentation and pan file parameters). It should raise a value error if these are not found. If this looks like a workable solution, should more tests be added for each case?