Skip to content
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

Remove orientation_strategy from Modelchain #1181

Merged
merged 4 commits into from
Mar 1, 2021

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Feb 28, 2021

  • Closes remove ModelChain.orientation_strategy #1028
  • 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.

Follow up to #1176 (comment). I went the lazy route and removed it without deprecation, but I am willing to not be lazy if a deprecation period seems like a good idea here.

@wholmgren wholmgren added the api label Mar 1, 2021
@wholmgren wholmgren added this to the 0.9.0 milestone Mar 1, 2021
Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Thanks @kanderso-nrel.

After some more thought, I'm leaning towards deprecating the parameter. The primary reason is that the default used to be south_at_latitude_tilt and users had to specify orientation_stragety=None to prevent ModelChain from modifying their systems (#290). As a long result of that bad API decision years ago, I still have modern code that sets the parameter to None. I'm guessing that at least a few other users might be in the same position.

FWIW, I briefly searched github and google for pvlib and south_at_latitude_tilt. I found one article in energies that used the parameter. I did not find any maintained code that uses it.

@wholmgren
Copy link
Member

Forgot to add... On the other hand, the changes to the object layer make the deprecation non-trivial, so unless you know a good way to do it then it's fine with me if we skip it. We're not a 1.0+ project for this reason.

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

LGTM. I'm ok removing without deprecation, in this case.

@wholmgren
Copy link
Member

Let's go ahead then. Thanks @kanderso-nrel

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 ModelChain.orientation_strategy
3 participants