pvlib-python
pvlib-python copied to clipboard
Rename `pvwatts` to `pvwattsv5` everywhere
- [x] Closes #1350
- [x] I am familiar with the contributing guidelines
- [x] Tests added
- [x] Updates entries in
docs/sphinx/source/reference
for API changes. - [x] 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`
). - [x] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
- [x] Pull request is nearly complete and ready for detailed review.
- [x] Maintainer: Appropriate GitHub Labels (including
remote-data
) and Milestone are assigned to the Pull Request and linked Issue.
A rather tedious PR, both to prepare and (I imagine) to review. $ git grep -n -e 'pvwatts[^v]'
is invaluable for finding stragglers.
For reference, SAM's implementation of PVWatts v8 is out (https://github.com/NREL/ssc/pull/630), but I don't think there's a publication about it yet, so let's implement v8 later and just focus on making room for it now. This PR is big enough already anyway.
After much fussing, this is ready for review. Assuming the general thrust of this PR is accepted, is 0.10 too soon for the removal dates?
I have no other comment on this PR. The interface (multiple functions pvwattsv5_xxx
) feels clumsy but so did the original. I support merging this.
For discussion: adding pvwattsv8_xxx
, pvwattsv10_xx
, etc. functions isn't appealing to me. What would alternatives be? Maybe a PVWatts
class with its own method e.g., PVWatts.dc
, and the version is set at initialization.
For discussion: adding pvwattsv8_xxx, pvwattsv10_xx, etc. functions isn't appealing to me.
Why not? If v10's xxx model is different from v8's I don't seen any reason not to implement both, and that naming scheme seems reasonable to me so long as the model is not taken from elsewhere (for example as temperature.fuentes
was). To be clear, I'm not suggesting we have both v8_xx
and v10_xx
if the xx model didn't change between v8 and v10.
I'm not saying that there isn't value is adding pvwattsv8_
etc., only that linearly increasing the number of pvwatts
functions isn't appealing.
And: if pvwattsv8_dc
is the same as pvwattsv5_dc
, is the user expected to know this? Or would we have a pvwattsv8_dc
that just wraps pvwattsv5_dc
so there's a complete set of pvwattsv8_
functions. That's got me thinking if there could be a better way to redesign this.
PVWatts seems to be converging towards a "SAM Lite" with many of the same models, so this proliferation might be asymptotic, but point taken.
My view: if the user wants to reimplement PVWatts themselves using pvlib's function layer, I don't mind them having to know that Version N of PVWatts needs a function named vM
(M!=N), just as they'd need to know to use iam.physical
and temperature.fuentes
for Version 5. I view the pvwattsv5_
function name prefix as identifying the original definition of the model, not all the places it has been used since then. If they want a full implementation of PVWatts v8 without having to know the constituent pieces, that's what ModelChain.with_pvwatts(version=8)
is for.
is 0.10 too soon for the removal dates?
We don't have a rule for deciding how many versions a deprecation lasts, right? I am starting to think 0.11 is better.
Since it's not clear that there will be a suitable length of time before 0.10, I have switched the deprecations to be scheduled for removal in 0.11 instead.
I'll leave this open for a couple days in case anyone wants to comment, otherwise I'll merge on Dec 14.
I think merging is fine and can appreciate being able to model the different versions of pvwatts with pvlib. Though, would be great with some documentation denoting what is needed to replicate a specific version.