Skip to content

remove arbitrary kwargs from Location, PVSystem, SingleAxisTracker, ModelChain #1034

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

Closed
wants to merge 2 commits into from

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Aug 27, 2020

  • Closes remove **kwargs from PVSystem, ModelChain, Location #1029
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

Straightforward except for dealing with the construction of LocalizedPVSystem and LocalizedSingleAxisTracker. The init methods of these classes passed all kwargs to both PVSystem and Location. I wrote some awful code to manually pull out the relevant values from the base object or kwargs. I'm sure you could do something smarter using inspect but that felt like overkill for objects that I don't use and don't want to support in the long run.

@wholmgren wholmgren added the api label Aug 27, 2020
@wholmgren wholmgren added this to the 0.8.0 milestone Aug 27, 2020
@kandersolar
Copy link
Member

LGTM.

don't want to support in the long run

Is there a timeline to deprecate and remove the LocalizedX classes? I ask because this isn't the first time I've seen not-very-positive remarks about them, but the classes persist regardless. Seems like if the plan is to remove them at some point, might as well do it sooner rather than later so it breaks less user code.

@wholmgren
Copy link
Member Author

No plan. I don't often see them in the google group or stack overflow, which suggests to me that they're not used much. They're usually not a problem to maintain along with the rest of the code, so I haven't been sufficiently bothered by their presence to put in the effort to argue for and implement their deprecation. I suspect that the best reason to remove them might be to simplify the documentation and API so that newcomers are not confused by another way of doing things.

@cwhanse
Copy link
Member

cwhanse commented Aug 27, 2020

I've never used the LocalizedX class, FWIW.

@cwhanse
Copy link
Member

cwhanse commented Aug 27, 2020

I would vote to remove the LocalizedX classes if only to avoid maintaining pvsystem._parse_localized_attributes

@mikofski
Copy link
Member

+1 remove localized, less is more

@wholmgren
Copy link
Member Author

wholmgren commented Aug 28, 2020

I searched github for instances of LocalizedPVSystem. It's not used much. The most notable is in rdtools:

https://github.com/NREL/rdtools/blob/8f00cd4b75ba5f69cd4057496a21aeeb51e8607b/rdtools/normalization.py#L183

@mdeceglie
Copy link
Contributor

Thanks for catching that @wholmgren. Removal of LocalizedPVSystem would break RdTools API. It's possible that we would be able to maintain backward compatibility by taking an optional Location as a parameter (thanks to @kanderso-nrel for the suggestion), but we haven't fully evaluated that. It would be a bit of a kludge though. In the long run (version 3 or later) we may deprecate sapm_dc_power because we want to leave pv modeling to pvlib, and encourage users toward normalize_with_expected_power().

@wholmgren
Copy link
Member Author

@mdeceglie I'm looking at deprecating the LocalizedPVSystem in 0.8 (next week or so) but waiting to remove it until 0.9 (at least a couple of months). We could wait until 0.10 if it would actually help rtools users, but I see the rdtools setup.py currently pins to pvlib 0.7. If you're going to continue restricting the pvlib version and then eventually deprecate sapm_dc_power, then I suspect we can make this change without surprising any users with broken code.

@wholmgren
Copy link
Member Author

started over in #1053

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.

remove **kwargs from PVSystem, ModelChain, Location
5 participants