Skip to content

Add loss input to example in modelchain.with_pvwatts docs #1863

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

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

AdamRJensen
Copy link
Member

  • [ ] Closes #xxxx
  • I am familiar with the contributing guidelines
  • [ ] Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

I had some difficulties figuring out what losses were used in the modelchain.with_pvwatts, I am making this PR to make it clearer and demonstrate how to change the loss factors.

My motivation for this PR is that I think it's important to encourage users to consider what loss assumptions are made and make it easier to modify them.

Also, it's typically new users who would use the modelchain.with_pvwatts model, which is why I have added the necessary imports such that the example can be run exactly as copied without having users to figure out where the different modules are imported from (something that would be difficult for a new user).

@AdamRJensen AdamRJensen changed the title Add loss input to modelchain.with_pvwatts docs Add loss input to example in modelchain.with_pvwatts docs Sep 16, 2023
@AdamRJensen AdamRJensen marked this pull request as ready for review September 16, 2023 14:32
Comment on lines +596 to +598
>>> 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}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have two examples here, one that keeps things as simple as possible, and one that gets more into the weeds (like specifying losses)? with_pvwatts is an easy point of entry for ModelChain, and PVWatts isn't really the model to use if you care a lot about the details, so I'm hesitant to make the only usage example more complicated.

IMHO there is also an argument to be made that PVWatts losses aren't really legitimate on their own and are better regarded as a single combined fudge factor, but maybe that's just my editorializing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think users should ever run a model without any considerations for what losses are specified or at least be aware that losses are accounted for by the model? Acknowledging and accepting the default values is perfectly resonable, but awareness of what they are seems pertinent.

Copy link
Member Author

@AdamRJensen AdamRJensen Sep 18, 2023

Choose a reason for hiding this comment

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

I have created two examples, one simple and one that builds upon the simple one where the losses are modified.

Let me know if it warrants a whatsnew entry (I don't think it does).

Comment on lines 587 to 588
The PVWatts model defaults to 14 % total system losses. The loss
assumptions can be modified as shown in the example below.
Copy link
Member Author

@AdamRJensen AdamRJensen Sep 18, 2023

Choose a reason for hiding this comment

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

Suggested change
The PVWatts model defaults to 14 % total system losses. The loss
assumptions can be modified as shown in the example below.
The PVWatts model defaults to 14 % total system losses. The loss
assumptions represent annual averages and can be modified as
shown in the example below.
The model is not recommended for high-accuracy applications.

Thoughts on this edit?

Copy link
Member

Choose a reason for hiding this comment

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

Two thoughts, neither of which I feel too strongly about:

  • I think "annual averages" is true but not as specific as it could be. How about "...represent constant DC-side loss fractions and can be modified..."? As an aside, constant losses means all simulation timesteps are affected equally (on a relative basis), so the "snow loss" applies to July just as much as it does to December...
  • Re "not recommended": I certainly don't disagree with the sentiment, but this struck me as too subjective a statement to be appropriate for pvlib's documentation. If we want to make novice users aware that PVWatts v5 isn't a very detailed model, I think it would be better to describe what the model does (or doesn't) do, and let the user judge suitability for themselves.

Copy link
Member

Choose a reason for hiding this comment

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

Bikeshedding: "The PVWatts losses are fractions of DC power and can be modified..."

@kandersolar kandersolar added this to the v0.10.2 milestone Sep 19, 2023
@kandersolar kandersolar merged commit ccfee19 into pvlib:main Sep 21, 2023
@AdamRJensen AdamRJensen deleted the pvwatts_high_losses_warning branch October 5, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants