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

[MAINT]: Decorator to warn about changed parameter names & allow for deprecation period #2237

Merged
merged 21 commits into from
Oct 15, 2024

Conversation

echedey-ls
Copy link
Contributor

@echedey-ls echedey-ls commented Oct 2, 2024

@AdamRJensen is threatening my mail inbox with issues reports from the parameter renaming in #2231 and #2235 (just kidding). So maybe instead of making breaking changes we can have some deprecation period.

Maybe over-engineering a pattern in each function for each rename is not the best choice (my own idea), but we can work out some decorator to do so.

Here is that decorator, instructions and recommendations on how to use it:

  • Introduced and tested a decorator that emits a warning when the old parameter name is used
    • It does NOT modify docs
    • It does NOT check in which version of pvlib to fail automatically, that's left to the test suite
    • For these non-features, I can try to do them if you are sure of what you want.
  • Example of edits required in pvlib.solarposition.hour_angle, docs down below.

Wiki?

In addition, I propose adding instructions on how to proceed with renamings in the wiki.

Docs

@echedey-ls echedey-ls marked this pull request as ready for review October 2, 2024 11:04
@echedey-ls
Copy link
Contributor Author

First thing is to consider if such a feature is needed/wanted for pvlib, feel free to weight in :)

@cwhanse
Copy link
Member

cwhanse commented Oct 2, 2024

Seems elegant and useful, and easy to remove if it proves troublesome.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

I am cautiously on board with the decorator :)

pvlib/tests/test__deprecation.py Outdated Show resolved Hide resolved
pvlib/tests/test__deprecation.py Show resolved Hide resolved
pvlib/tests/test__deprecation.py Show resolved Hide resolved
Comment on lines 1349 to 1356
times : :class:`pandas.DatetimeIndex`
time : :class:`pandas.DatetimeIndex`
Corresponding timestamps, must be localized to the timezone for the
``longitude``.

.. versionchanged:: 0.11.2
Renamed from ``times`` to ``time``.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if the docs somehow indicated that times is still allowed, for now. Maybe we should we use deprecated instead of versionchanged during the deprecation period, and versionchanged afterwards?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a point to state that it's still allowed. The less work the better so I vote for version changed only. Deprecating is already cumbersome and I'm hesitant to touch that require it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My line of thought was exactly the same of @AdamRJensen 's. However, I did hesitate about the message. It can be changed to something like:

.. versionchanged:: 0.12.0
    Renamed from ``times`` to ``time``. ``times`` was deprecated in ``0.11.2``.

or

.. versionchanged:: 0.11.2
    Renamed from ``times`` to ``time``. ``times`` will be removed in ``0.12.0``.

which explains both things but clutters the docs IMO.

I don't mind changing it thou.

Copy link
Contributor Author

@echedey-ls echedey-ls Oct 2, 2024

Choose a reason for hiding this comment

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

But it's true that in v0.12.0 a follow-up PR to clean up the test that I have pushed now in 65e144b is required

Edit to complete the statement: a PR required that can also be used to clean up what is left and modify docs as needed.

echedey-ls and others added 5 commits October 2, 2024 21:37
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
@echedey-ls
Copy link
Contributor Author

I'd say this is now ready to choose a normalized deprecation/versionchanged directive format and discuss how to proceed in the future removal, or keep the admonitions.

I have a slight preference for what's been done at sun_rise_set_transit_spa, and just remove the parameter deprecated directive in v0.12.

pvlib/solarposition.py Outdated Show resolved Hide resolved
@echedey-ls
Copy link
Contributor Author

Let me know if new actions are desired in this PR, or if you prefer to split add decorator / address issues in different PRs.

Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

I think this is awesome!

pvlib/solarposition.py Outdated Show resolved Hide resolved
echedey-ls and others added 2 commits October 15, 2024 15:49
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-Authored-By: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

@echedey-ls I hate to ask for more work, but I think we should limit the scope of this PR to only creating the decorator (not applying it to real pvlib functions). Would you mind splitting out the solarposition.py parameter renames into new PR(s)? That will give them better visibility.

It was helpful to see the decorator "in action" on real examples though :)

@echedey-ls
Copy link
Contributor Author

No problem. It doesn't make much sense to open the other PR now without the decorator on main, so I'll leave that up to you. These branch's commits already have the necessary changes to cherrypick them or just start from scratch, they aren't that many lines.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Thanks @echedey-ls!

@kandersolar kandersolar merged commit b6ac5a1 into pvlib:main Oct 15, 2024
27 checks passed
@echedey-ls echedey-ls deleted the renamed-param-deprecation-deco branch October 20, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants