-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Deprecate modelchain.get_orientation
#2495
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
base: main
Are you sure you want to change the base?
Conversation
I could also add tests to check whether the correct values are returned according to the strategy provided, is this necessary?
|
Optional IMO. I'm actually wondering why modelchain.py has this function. It was added in 2016 in the initial modelchain build and hasn't been used since in the |
@cwhanse fair observation, are you proposing we deprecate and remove the function entirely? |
Yes get rid of it! |
get_orientation
test to check error message stringmodelchain.get_orientation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Just a comment and a formatting preference down below.
Functions | ||
--------- | ||
|
||
Functions for power modeling. | ||
|
||
.. autosummary:: | ||
:toctree: generated/ | ||
|
||
modelchain.get_orientation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly following deprecations as a concept, this shouldn't be removed till the removal - a just in case clarification. Anyway, we all are sure this function hasn't found it's way into many codebases so I agree with removing it here.
@@ -10,6 +10,8 @@ Breaking Changes | |||
|
|||
Deprecations | |||
~~~~~~~~~~~~ | |||
* Deprecate :py:func:`~pvlib.modelchain.get_orientation`. Removal scheduled for | |||
0.14.0. (:pull:`2691`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.14.0. (:pull:`2691`) | |
``v0.14.0``. (:pull:`2691`) |
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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.See discussion in related #2492Original testtest_bad_get_orientation():
only checked whether a value error was raised, the new test (with what I think is a clearer name) checks not only whether a value error is raised but also whether the error string matches