Skip to content

pvfactors_timeseries docstring types are incorrect #1332

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

Closed
kandersolar opened this issue Oct 28, 2021 · 4 comments · Fixed by #1361
Closed

pvfactors_timeseries docstring types are incorrect #1332

kandersolar opened this issue Oct 28, 2021 · 4 comments · Fixed by #1361
Labels
Milestone

Comments

@kandersolar
Copy link
Member

kandersolar commented Oct 28, 2021

Describe the bug
The pvfactors_timeseries docstring currently lists several parameters as "numeric", implying scalar values are acceptable (docs). However, some parameters cannot actually be scalars and should be listed as "array-like" instead.

To Reproduce
Example showing that scalar inputs for the module orientation parameters are not acceptable:

Click to expand!
import pvlib
import pandas as pd
from pvlib.bifacial import pvfactors_timeseries

times = pd.date_range('2019-01-01', '2019-01-02', freq='15min').tz_localize('Etc/GMT+5')
location = pvlib.location.Location(40, -80)
solarpos = location.get_solarposition(times)
clearsky = location.get_clearsky(times)

kwargs = dict(
    solar_azimuth=solarpos['azimuth'],
    solar_zenith=solarpos['apparent_zenith'],
    axis_azimuth=0,
    timestamps=times,
    dni=clearsky['dni'],
    dhi=clearsky['dhi'],
    gcr=0.3,
    pvrow_height=1.5,
    pvrow_width=1.0,
    albedo=0.2
)

pvfactors_timeseries(surface_tilt=20, surface_azimuth=90, **kwargs)  # IndexError: too many indices for array
pvfactors_timeseries(surface_tilt=pd.Series(20, index=times),
                     surface_azimuth=pd.Series(90, index=times),
                     **kwargs)  # works

Expected behavior
Correct the types in the docstring to accurately reflect pvfactors' requirements.

Additional context
Originally discussed here: #1331 (reply in thread)
Edit: this is more or less a duplicate of #1127

@campanelli-sunpower
Copy link

@kanderso-nrel My understanding is that array-like should accommodate scalars, in which case I think this means pvfactors is advertising falsely. Thoughts?

https://numpy.org/devdocs/reference/typing.html#numpy.typing.ArrayLike

@kandersolar
Copy link
Member Author

Oh right, I had forgotten about the inconsistency between how pvlib and numpy each define array-like. #765 is very relevant. Maybe we call it array-like for now until we change the pvlib convention later? I'd also be fine with array or Series, or something like that.

@kandersolar
Copy link
Member Author

Maybe this is less a docs bug and more a functionality bug. I generally expect pvlib functions to work with scalar orientation values. A better fix may be to allow scalar inputs and to cast scalars to arrays inside the function prior to forwarding them on to pvfactors.

@cwhanse
Copy link
Member

cwhanse commented Dec 17, 2021

A better fix may be to allow scalar inputs and to cast scalars to arrays inside the function prior to forwarding them on to pvfactors.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants