pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

Rename `pvwatts` to `pvwattsv5` everywhere

Open kandersolar opened this issue 2 years ago • 1 comments

  • [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.

kandersolar avatar Sep 22 '22 22:09 kandersolar

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?

kandersolar avatar Sep 28 '22 18:09 kandersolar

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.

cwhanse avatar Nov 03 '22 16:11 cwhanse

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.

kandersolar avatar Nov 03 '22 16:11 kandersolar

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.

cwhanse avatar Nov 03 '22 16:11 cwhanse

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.

kandersolar avatar Nov 03 '22 17:11 kandersolar

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.

kandersolar avatar Dec 02 '22 19:12 kandersolar

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.

kandersolar avatar Dec 12 '22 20:12 kandersolar

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.

AdamRJensen avatar Dec 14 '22 13:12 AdamRJensen