Skip to content

Rename pvwatts to pvwattsv5 everywhere #1558

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 18 commits into from
Dec 14, 2022
Merged

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Sep 22, 2022

  • Closes account for major pvwatts changes #1350
  • 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.

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 (NREL/ssc#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 kandersolar added this to the 0.9.4 milestone Sep 22, 2022
@kandersolar kandersolar marked this pull request as ready for review September 28, 2022 18:14
@kandersolar
Copy link
Member Author

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?

@cwhanse
Copy link
Member

cwhanse commented Nov 3, 2022

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.

@kandersolar
Copy link
Member Author

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.

@cwhanse
Copy link
Member

cwhanse commented Nov 3, 2022

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.

@kandersolar
Copy link
Member Author

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
Copy link
Member Author

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 kandersolar mentioned this pull request Dec 9, 2022
9 tasks
@kandersolar
Copy link
Member Author

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.

@AdamRJensen
Copy link
Member

AdamRJensen commented Dec 14, 2022

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.

@kandersolar kandersolar merged commit e6e5b04 into pvlib:main Dec 14, 2022
@kandersolar kandersolar deleted the pvwattsv5 branch December 14, 2022 14:36
kandersolar added a commit to kandersolar/pvlib-python that referenced this pull request Dec 20, 2022
kandersolar added a commit that referenced this pull request Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

account for major pvwatts changes
3 participants