-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Repair broken link in get_pvgis_hourly
documentation
#2517
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
pvlib/modelchain.py
Outdated
@@ -447,7 +447,7 @@ def with_pvwatts(cls, system, location, | |||
|
|||
>>> pvwatts_losses = {'soiling': 2, 'shading': 3, 'snow': 0, 'mismatch': 2, | |||
>>> 'wiring': 2, 'connections': 0.5, 'lid': 1.5, | |||
>>> 'nameplate_rating': 1, 'age': 0, 'availability': 30} | |||
>>> 'nameplate_rating': 1, 'age': 0, 'availability': 3} |
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 see how 30 might have meant 3, but now these add up to 15% rather than the 14% stated in the docstring, so should this be 2 rather than 3, or is something else (the docstring?) wrong here?
@cwhanse @kandersolar ?
>>> 'nameplate_rating': 1, 'age': 0, 'availability': 3} | |
>>> 'nameplate_rating': 1, 'age': 0, 'availability': 2} |
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.
Had the same question myself @RDaxini ahaha
Had to google it to remind myself that the loss factors are not additive but multiplicative. So if you do (1-loss_factor) for each source and then multiply all of them you get something like 0.859 (i.e., 14% overall loss)
I thought of adding a remark on this to the documentation, but then I thought reviewers could find it a bit unnecessary....
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 for clarifying, that's my mistake! You can mark this conversation as resolved, and then one of the other maintainers will approve workflows and double check the PR before merging.
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.
Indeed PVWatts losses combine multiplicatively; see the source code of pvlib.pvsystem.pvwatts_losses
.
Regarding the typo: from the comment immediately above the dict, it looks like those parameter values are not supposed to reflect the defaults but rather an example set of user-specified values. I think the 30 was intentional then, and it defeats the purpose of the example to change it back to the default value :)
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.
Agree with @kandersolar remark. Can you confirm if the workflow is what I should do now?
- revert the typo that wasn't a typo, commit locally and then push to my repository
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.
- revert the typo that wasn't a typo, commit locally and then push to my repository
Sounds correct to me:)
You can make the change manually in the file, or I think there is a function in git to revert a specific commit too. Either is fine.
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.
@RDaxini @kandersolar should be good to go :-)
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.
My bad, thanks for clarifying. And, thanks for this fix.
Let's add a whatsnew entry and then I think we're good to go.
@RDaxini, once my PR is accepted should I directly edit it in pvlib's repository? |
No need, another maintainer (with write access) will use another git feature to merge your changes into the main repository—like accepting tracked changes on a word document someone else has edited. This usually happens after a couple of approvals. |
Sorry, my bad. I actually meant the whatsnew entry. Is that also on the maintainer? |
Oh right, no you can edit the file yourself on your branch and push the changes to this PR. Side context: |
Thanks for pushing for this. But following the initial premise, I will leave my |
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 @ramaroesilva!
get_pvgis_hourly
documentation
docs/sphinx/source/reference
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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Repairs broken link in
pvlib.iotools.get_pvgis_hourly
identified #2508.