Deprecate `modelchain.get_orientation` #2495

RDaxini · 2025-07-02T21:59:56Z

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 · 2025-07-02T22:10:47Z

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 · 2025-07-02T22:18:56Z

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 · 2025-07-03T15:03:54Z

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

wholmgren · 2025-07-03T15:14:29Z

Yes get rid of it!

echedey-ls

LGTM :)
Just a comment and a formatting preference down below.

docs/sphinx/source/reference/modelchain.rst

docs/sphinx/source/whatsnew/v0.13.1.rst

kandersolar · 2025-07-07T16:33:10Z

docs/sphinx/source/whatsnew/v0.13.1.rst

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


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!

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

I like setting a version more for us, so it does not get forgotten. I understand Kevin's point is to make it more agile.
So I propose that instead of an specific semver, we set it to removal="next version". That's agile, and concrete for users and us.

>>> import pvlib.modelchain as mc >>> mc.get_orientation('flat') <stdin>:1: pvlibDeprecationWarning: The pvlib.modelchain.get_orientation function was deprecated in pvlib 0.13 and will be removed in next version. (0, 180) >>>

And let's set the patch number too, so it's deprecated in 0.13.x and next version clearly is 0.13.(x+1).

I think many people (me included) would interpret "next version" to include patches, so the next version after 0.13.1 would be 0.13.2 (or 0.14.0, if there is no 0.13.2). So I don't think "next version" solves the problem in my comment above :(

I like setting a version more for us, so it does not get forgotten.

I agree, but I think the deprecation tracker is the best tool for that :)

I had the same thought so I followed up with an off-github conversation with Echedey to clarify, I thought I might be the only one to have misunderstood 😅 I think the idea was to say something so there is a message, but without committing to something specific? @echedey-ls may be able to clarify further.

I agree, but I think the deprecation tracker is the best tool for that :)

Makes sense, so is the suggestion here just to stick with no removal statement (whatever the default is) and then make sure we all stay on top of the deprecation tracker? (Thanks for creating that @echedey-ls!) --what is the consensus?

stick with no removal statement (whatever the default is) and then make sure we all stay on top of the deprecation tracker

This has my vote!

FYI the default is "soon":

pvlib-python/pvlib/_deprecation.py

Lines 141 to 142 in 908d3da

if removal == "":

removal = "soon"

I think many people (me included) would interpret "next version" to include patches, so the next version after 0.13.1 would be 0.13.2 (or 0.14.0, if there is no 0.13.2). So I don't think "next version" solves the problem in my comment above :(

I think that does address what I interpret the problem is, that is not declaring we are going to be agile in the removal that will happen in whatever next version is. You are right that for ensuring removals get done we have the deprecation tracker, my bad for a shitty argument.

My reasons to still support "next" do not make for a strong opinion:

"soon" is not precise in the same way "next" is. And precedences of "soon" removals meant 2+ minor versions. I think we should strive for super clear declaration of intentions. And being so does not look harmful in this situation.

removals in patch versions are against pvlib-flavoured semver which usually happens at minor versions. I would just heavily differentiate this edge-deprecation and removal from any other, to not mislead (new) users with the usual procedures involved in pvlib. Not all users may be aware of the deprecations tracker.

I'm in favour of removing this one in a patch version, thou. Unless anybody else objects, please go ahead with the current choice @RDaxini , I don't want to block such a small change.

pvlib/modelchain.py

tests/test_modelchain.py

…thon into modelchain_test

RDaxini · 2025-07-18T17:01:11Z

@echedey-ls @kandersolar thanks for the feedback about the deprecation scheduling. Please review, let me know if the current version is in line with what you guys have in mind.
I agree with keeping something there for maintainers not to lose track of the issue, and retaining some flexibility in the removal. @echedey-ls let's update the deprecations tracker issue with this PR too.

enhance tests to check error message

4653d9d

RDaxini added enhancement testing labels Jul 2, 2025

RDaxini added this to the v0.13.1 milestone Jul 2, 2025

linting, whatsnew

442ee6a

RDaxini added 4 commits July 3, 2025 09:49

deprecate get_orientation

7b7f667

remove test

2a7ac35

Update modelchain.rst

8e25637

Update v0.13.1.rst

bb3d7df

RDaxini changed the title ~~Enhance get_orientation test to check error message string~~ Deprecate modelchain.get_orientation Jul 3, 2025

RDaxini added deprecation Use for issues and PRs which involve deprecations api and removed enhancement testing labels Jul 3, 2025

cwhanse approved these changes Jul 3, 2025

View reviewed changes

RDaxini mentioned this pull request Jul 3, 2025

Deprecations Tracker #2396

Open

echedey-ls approved these changes Jul 4, 2025

View reviewed changes

docs/sphinx/source/reference/modelchain.rst Outdated Show resolved Hide resolved

docs/sphinx/source/whatsnew/v0.13.1.rst Outdated Show resolved Hide resolved

Update v0.13.1.rst

e2c7fc3

kandersolar reviewed Jul 7, 2025

View reviewed changes

RDaxini modified the milestone: v0.13.1 Jul 7, 2025

RDaxini and others added 4 commits July 16, 2025 10:25

reinstate test

002d782

Merge branch 'main' into modelchain_test

aef91b7

update removal schedule

78b71e7

Merge branch 'modelchain_test' of https://github.com/RDaxini/pvlib-py…

414f8ca

…thon into modelchain_test

reinstate function in docs

1490b30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Deprecate `modelchain.get_orientation` #2495

Deprecate `modelchain.get_orientation` #2495

RDaxini commented Jul 2, 2025 •

edited

Loading

Uh oh!

RDaxini commented Jul 2, 2025

Uh oh!

cwhanse commented Jul 2, 2025

Uh oh!

RDaxini commented Jul 3, 2025

Uh oh!

wholmgren commented Jul 3, 2025

Uh oh!

echedey-ls left a comment

Uh oh!

Uh oh!

Uh oh!

kandersolar Jul 7, 2025

Uh oh!

RDaxini Jul 7, 2025

Uh oh!

echedey-ls Jul 9, 2025

Uh oh!

kandersolar Jul 18, 2025 •

edited

Loading

Uh oh!

RDaxini Jul 18, 2025 •

edited

Loading

Uh oh!

kandersolar Jul 18, 2025

Uh oh!

echedey-ls Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RDaxini commented Jul 18, 2025

Uh oh!

Uh oh!

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

Deprecate modelchain.get_orientation #2495

Are you sure you want to change the base?

Deprecate modelchain.get_orientation #2495

Conversation

RDaxini commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RDaxini commented Jul 2, 2025

Uh oh!

cwhanse commented Jul 2, 2025

Uh oh!

RDaxini commented Jul 3, 2025

Uh oh!

wholmgren commented Jul 3, 2025

Uh oh!

echedey-ls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kandersolar Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

RDaxini Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

echedey-ls Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

kandersolar Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RDaxini Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kandersolar Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

echedey-ls Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RDaxini commented Jul 18, 2025

Uh oh!

Uh oh!

Deprecate `modelchain.get_orientation` #2495

Deprecate `modelchain.get_orientation` #2495

RDaxini commented Jul 2, 2025 •

edited

Loading

kandersolar Jul 18, 2025 •

edited

Loading

RDaxini Jul 18, 2025 •

edited

Loading