`scaling.py` Numpy function arg bug

[vfilter]

Describe the bug

pvlib-python/pvlib/scaling.py

Line 199 in cc75255

pos = coordinates * np.array(m_per_deg_lat, m_per_deg_lon)

has a bug where longitude gets passed as a second arg to np.array. The second arg for np.array is the dtype however.

pos = coordinates * np.array(m_per_deg_lat, m_per_deg_lon)

What will happen with the current implementation is that m_per_deg_lat will be silently used as a multiplier for both axes.

# resulting broadcast
lat * m_per_deg_lat
lon * m_per_deg_lat   # <- this is wrong. it should be using the lon instead

Expected behavior
Correct implementation is as follows:

pos = coordinates * np.array([m_per_deg_lat, m_per_deg_lon])

This could also affect the unit tests, but I didn't check.

[vfilter]

As a comment, and I'm sure this has been discussed before, but has the pvlib community until now ever considered using Python typings to avoid these kind of issues? Given the recent advances in Python tooling with uv and ty making their entry, it seems that maybe using explicit typing could prevent bugs like this from happening.

edit: Thinking about this a bit more, given that pvlib is using Numpy here, which infamously isn't making great use of Python typing either, the question might be a bit moot in this case, but holds true more generally I think.

[markcampanelli]

numpy has a typing module, which I think is getting better. I think the fundamental problem is that numpy.ndarray is such a "Swiss army knife" of a class, on top of Python's already loosey-goosey type system.

For pvlib-python discussion of typing, see #2062

Re: the issue reported here, I'm wondering how it slipped through the tests.

[cwhanse]

Confirmed. It is some consolation that latlon_to_xy is a utility function that is not used within pvlib.

Part of my resistance to using typing are docstrings like this one for ModelChainResult

[markcampanelli]

Confirmed. It is some consolation that latlon_to_xy is a utility function that is not used within pvlib.

Part of my resistance to using typing are docstrings like this one for ModelChainResult

Yikes! I don't see any public methods here, so this is where it might be better to use a TypedDict or a @DataClass, but I'd have to run that experiment to see how it might render better (or worse!) in the docs or an IDE.

[cwhanse]

Re: the issue reported here, I'm wondering how it slipped through the tests.

Test are all at low latitudes, so meter per degree longitude is nearly the same as meter per degree latitude. And the test resolution is only checking agreement to decimal=1 which means the tests pass if values agree in the first digit. Almost certainly a misunderstanding of what decimal means.

[markcampanelli]

@cwhanse In my VSCode IDE, the docstring popup for ModelChainResult looks pretty good:

For Sphinx/RTD, I think there are ways of turning off the type hints in the signatures:

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_typehints

[r0hansaxena]

hey @vfilter , I've opened a PR for this - #2712 , would love your feedback!