pvsystem.Array.get_irradiance raises an error if solar_zenith is passed as a float

[lucasschn]

Bug Description
pvsystem.get_irradiance documentation says solar_zenith can be passed as a float or a Series. However, if dni_extra is not defined and solar_zenith is passed as float, then solar_zenith.index is not defined and the call to irradiance.get_extra_irradiance with solar_zenith.index as an argument fails.

To Reproduce
Steps to reproduce the behavior:
test_sys = pvsystem.PVSystem()
test_sys.get_irradiance(0.0, 0.0, 0.0, 0.0, 0.0)

Error message
AttributeError: 'float' object has no attribute 'index'

Expected behavior
Return a DataFrame containing the irradiance components, as specified in the documentation.

Possible solutions

1. Ask for datetime if dni_extra is not passed as argument
2. Remove float from supported types for solar_zenith

Versions:

• pvlib.__version__: 0.9.0
• pandas.__version__: 1.3.3
• python: 3.9

[cwhanse]

Confirmed bug. Thanks @lucasschn.

The PVSystem.get_irradiance method is a convenience wrapper for irradiance.get_total_irradiance which is common front end for the set of sky diffuse models, some of which require dni_extra. Currently, PVSystem.get_irradiance uses solar_zenith.index to calculate dni_extra if the user doesn't provide dni_extra.

Here's a few options I see:

1. change the PVSystem.get_irradiance docstring to specify that solar_zenith (and solar_azimuth, for consistency) be Series. Not sure I like this but I like the following less:
2. inspect all the potential Series inputs for an index and use the first one found. Raise an error if none are found.
3. require dni_extra as input if solar_zenith isn't a Series
4. add datetime as a new parameter (trend has been to remove this kind of parameter).

I think the important goal is that PVSystem.get_irradiance remains convenient to use.

[lucasschn]

Thanks @cwhanse for the feedback. I personally find Option 3 (require dni_extra as input if solar_zenith isn't a Series) the most elegant. I could implement it and open a PR if this solution is indeed the preferred one.

[kandersolar]

Quibble: technically it is Array.get_irradiance (not PVSystem.get_irradiance) that calculates dni_extra if it is missing:

pvlib-python/pvlib/pvsystem.py

Lines 1468 to 1469 in 90013a5

	if dni_extra is None:
	dni_extra = irradiance.get_extra_radiation(solar_zenith.index)

I think I also lean towards option 3. I think changing that condition to dni_extra is None and hasattr(solar_zenith, 'index') would result in ValueError: dni_extra is required for model haydavies, which seems ok to me.

Just curious, what's the use case for passing scalars? I personally can't recall a case where I've used anything other than Series.

[lucasschn]

@kanderso-nrel You are right in pointing out that the invoked function is Array.get_irradiance, but it's not possible to instantiate a simple Array with array=pvsystem.Array(), as a mount needs to be defined. That's why I used the wrapper PVSystem.get_irradiance in my simple example for reproducing the bug.

About the use case, I passed scalars whereas I was exploring the module and testing it in a Jupyter notebook, without making real calculations. I can imagine it's not a typical use case but I believe that if the documentation says it is a supported case, then it should really be :)

[adriesse]

Most or all of the bottom layer functions work with scalars and I think that's important. May be less so for the classes, but still desirable.

[lucasschn]

In order to give clearer inputs to the user, I suggest to implement the following:

if dni_extra is None:
            if isinstance(solar_zenith, pd.Series):
                dni_extra = irradiance.get_extra_radiation(solar_zenith.index)
            elif isinstance(solar_zenith, (int, float)):
                raise ValueError('If solar_zenith is passed as a float, then dni_extra must be defined.')
            else:
                raise TypeError(f'Argument solar_zenith must be a float or a pandas Series. Currently is a {type(solar_zenith)}.')

[lucasschn]

Invalid inputs

If solar_zenith is neither a float nor a pandas Series, an error is raised:

pv_sys.get_irradiance(solar_zenith='hello', solar_azimuth=0, dni=12, ghi=43, dhi=12)

TypeError: Argument solar_zenith must be a float or a pandas Series. Currently is a <class 'str'>.

If solar_zenith is passed as a float, dni_extra must be defined and an error is raised:

pv_sys.get_irradiance(solar_zenith=0, solar_azimuth=0, dni=12, ghi=43, dhi=12)

ValueError: If solar_zenith is passed as a float, then dni_extra must be defined.

Valid inputs

If solar_zenith and dni_extra are defined, the function returns an OrderedDict:

pv_sys.get_irradiance(solar_zenith=0, solar_azimuth=0, dni=12, ghi=43, dhi=12, dni_extra=10)

OrderedDict([('poa_global', 24.0),
             ('poa_direct', 12.0),
             ('poa_diffuse', 12.0),
             ('poa_sky_diffuse', 12.0),
             ('poa_ground_diffuse', 0.0)])

If solar_zenith is passed as a pandas series, the function returns a pandas DataFrame:

tz = 'Europe/Zurich'
lat, lon = 46.2, 6.15

times = pd.date_range('2020-01-01 00:00:00', '2021-01-01', closed='left',
                      freq='H', tz=tz)
solpos = solarposition.get_solarposition(times, lat, lon)
pv_sys.get_irradiance(solar_zenith=solpos.zenith, solar_azimuth=0, dni=12, ghi=43, dhi=12)

	poa_global	poa_direct	poa_diffuse	poa_sky_diffuse	poa_ground_diffuse
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
...	...	...	...	...
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0
11.89816	0.0	11.89816	11.89816	0.0

[lucasschn]

Is that the expected behavior?
If yes, then the documentation still needs a small update: the function can also return an OrderedDict, not only a pandas DataFrame.

[cwhanse]

How about

if dni_extra is None:
    try:
        dni_extra = irradiance.get_extra_radiation(solar_zenith.index)
    except AttributeError:
         raise ValueError('If dni_extra is None then solar_zenith must be a Series.')

My hesitation is that AttributeError could occur in other ways than solar_zenith missing an index.

I agree, the docstring should communicate that an OrdereDict can be returned.

[wholmgren]

*must be a Series with a DatetimeIndex.

I also like @kanderso-nrel's suggestion in #1338 (comment)

[lucasschn]

@cwhanse looking at the documentation about Python's built-in exceptions, I don't see any other case where an AttributeError could be raised, other than solar_zenith.index not being defined. And if solar_zenith is a pandas.Series, it must have an index. Am I missing something? Do you see any other caveat?

I think there is also a risk that solar_zenith has an attribute named index without necessarily being a pandas.Series.

[lucasschn]

@wholmgren Yes, you are right. What about this:

if isinstance(solar_zenith, pd.Series) and isinstance(solar_zenith.index, pd.DatetimeIndex):
            if dni_extra is None:
                dni_extra = irradiance.get_extra_radiation(solar_zenith.index)
        elif isinstance(solar_zenith, (int, float)):
            if dni_extra is None:
                raise ValueError('If solar_zenith is passed as a float, then dni_extra must be defined.')
        else:
            raise TypeError(f'Argument solar_zenith must be a float or a pandas Series with Datetime index. Currently is a {type(solar_zenith)} and has index of type {solar_zenith.index.dtype}.')

[lucasschn]

About @kanderso-nrel's suggestion in #1338 (comment), indeed hasattr(solar_zenith, 'index') could be an additional check, but I don't think it replaces isinstance(solar_zenith, pandas.Series), because (as already mentioned above):

I think there is also a risk that solar_zenith has an attribute named index without necessarily being a pandas.Series.

Therefore I would say that isinstance(solar_zenith, pandas.Series is more robust than hasattr(solar_zenith, 'index'), but of course we could put both checks:

isinstance(solar_zenith, pd.Series) and hasattr(solar_zenith, 'index') and isinstance(solar_zenith.index, pd.DatetimeIndex)

Do you guys see a case where it could be better to user hasattr only?

[adriesse]

You could perhaps just use the solar constant as dni_extra when all else fails. Then you don't need to raise anything.