Refactor duplicated OrderedDict/DataFrame return logic in clearsky models

[aman-coder03]

While reading clearsky.py, I noticed this TODO comment in bird():

TODO: be DRY, use decorator to wrap methods that need to return either
OrderedDict or DataFrame instead of repeating this boilerplate code

A few clearsky model functions (ineichen, simplified_solis, bird) repeat the same pattern of returning either an OrderedDict or a DataFrame depending on the input type.

It might be cleaner to introduce a small decorator to handle this logic and remove the duplicated boilerplate.

Happy to work on this if it sounds good.

[echedey-ls]

I think this makes sense for pvlib in general. That was a good point brought in #276 by @mikofski , seems like it aged for a bit!

Let's wait for other maintainers opinion before opening a PR @aman-coder03 .

I suggest the decorator inspects the type of all or a subset of parameters, and if any is of a sub-pandas type, wrap the result as a dataframe. Some (if not most) boilerplate implementations use a pattern where only one input type is tested. This behaviour can be kept for backwards compat by specifying the input parameter to check as a decorator argument. If checking the type of all parameters, that may be considered a breaking change for a minor release.

For context too:

• is there still a need for irradiance functions to return OrderDict since Python-3.7 is minimum version? #1684

[kandersolar]

Maybe taking an inventory of where this pattern occurs more broadly (not just in clearsky.py) would be good, to understand if a general solution in tools.py would require anything extra.

Anyway, there is a precedent for a "clean up function output type" decorator: _unwrap_single_value in pvsystem.py. Generally though, we've been pretty avoidant of decorators. They are elegant and concise to those who are comfortable using them, and bizarre and indecipherable otherwise. Many (most?) pvlib contributors are in the latter camp, so it's worth considering keeping it simple and calling some kind of _maybe_to_dataframe(out, inputs) at the end of every function instead of using a decorator.

[williamhobbs]

FWIW, I have little to no idea what decorators are. Then the first result of google search for "python decorator" is a short Stack Overflow answer [1] that points to another roughly 16 page SO answer [2], so I'm a little intimidated and apparently in the "latter camp" mentioned by Kevin....

[Edit: but I should add that I'm not at all opposed to learning about decorators if others in the community say it's worthwhile.]

[1] https://stackoverflow.com/a/12046984
[2] https://stackoverflow.com/a/1594484

[echedey-ls]

I can undoubtedly agree with that it's the most complicated abstraction in Python. Thou I think all the team members are able to learn about them without much time overhead and it's beneficial as long as it makes code more readable. However, I retract my original suggestion to use decorators for the following reason: some functions [may] return tuples and we may want to apply DataFrame conversion just for one of the elements. Implementing a decorator that works with that would be unreadable.

Edit: +1 for _maybe_to_dataframe / _collection_as_inputs / ...

[aman-coder03]

A small helper like _maybe_to_dataframe seems like a clearer compromise, given the goal of keeping things readable for a broad contributor base. It would centralize the logic without introducing additional abstraction layers.