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 all 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.
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.