Skip to content

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

RDaxini
Copy link
Member

@RDaxini RDaxini commented Jul 2, 2025

  • Related discussion Fix typos in ModelChain error messages #2492
  • I am familiar with the contributing guidelines
  • Tests added
  • 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.

See discussion in related #2492

Original test test_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

@RDaxini RDaxini added this to the v0.13.1 milestone Jul 2, 2025
@RDaxini
Copy link
Member Author

RDaxini commented Jul 2, 2025

I could also add tests to check whether the correct values are returned according to the strategy provided, is this necessary?
For example:

def test_get_orientation_flat():
    surface_tilt, surface_azimuth = modelchain.get_orientation('flat')
    assert surface_tilt == 0
    assert surface_azimuth == 180

@cwhanse
Copy link
Member

cwhanse commented Jul 2, 2025

tests to check whether the correct values are returned

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 ModelChain methods. There's a PVSystem.get_orientation method but that looks like just casting to a dict.

@RDaxini
Copy link
Member Author

RDaxini commented Jul 3, 2025

@cwhanse fair observation, are you proposing we deprecate and remove the function entirely?

@wholmgren
Copy link
Member

Yes get rid of it!

@RDaxini RDaxini changed the title Enhance get_orientation test to check error message string Deprecate modelchain.get_orientation Jul 3, 2025
@RDaxini RDaxini added deprecation Use for issues and PRs which involve deprecations api and removed enhancement testing labels Jul 3, 2025
@RDaxini RDaxini mentioned this pull request Jul 3, 2025
Copy link
Contributor

@echedey-ls echedey-ls left a 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.

Comment on lines +13 to +14
* Deprecate :py:func:`~pvlib.modelchain.get_orientation`. Removal scheduled for
``v0.14.0``. (:pull:`2691`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Deprecate :py:func:`~pvlib.modelchain.get_orientation`. Removal scheduled for
``v0.14.0``. (:pull:`2691`)
* Deprecate :py:func:`~pvlib.modelchain.get_orientation`. (:pull:`2691`)

In case v0.14.0 turns out to be in the next few releases, I think it is better to not mention a version number.

However, I notice that generally I seem to be the only one advocating for not mentioning specific versions regarding deprecation removals. Maybe it's time to discuss and adopt an actual policy here? In the meantime, feel free to overrule me!

Copy link
Member Author

Choose a reason for hiding this comment

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

In case v0.14.0 turns out to be in the next few releases, I think it is better to not mention a version number.

Hmm that's a fair point, I confess I had not thought much about that. I think I am on board with your suggestion. If no-one objects in the next day or two, I'll commit your suggestion.

Maybe it's time to discuss and adopt an actual policy here?

+1

@@ -59,6 +61,13 @@
)


@deprecated(
since="0.13",
removal="0.14",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
removal="0.14",

@@ -59,6 +61,13 @@
)


@deprecated(
since="0.13",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
since="0.13",
since="0.13.1",

Comment on lines -1788 to -1792
def test_bad_get_orientation():
with pytest.raises(ValueError):
modelchain.get_orientation('bad value')


Copy link
Member

Choose a reason for hiding this comment

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

To add to @echedey-ls's point above, the usual practice would be to wait to remove the tests until we remove the functionality. For the deprecation period, we would usually retain the tests, just slightly modified so that the function calls are inside with pytest.warns(pvlibDeprecationWarning, match="..."): blocks to (1) ensure the deprecation warning is working as expected and (2) prevent that warning from escaping into the test suite's logging output.

I suggest just for the sake of best practice we should do that here too.

@RDaxini RDaxini modified the milestone: v0.13.1 Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api deprecation Use for issues and PRs which involve deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants