Function ghi_from_poa_driesse_2023 cannot have None default value for dni_extra optional parameter

[iblasi]

Describe the bug
The function ghi_from_poa_driesse_2023 defines the parameter dni_extra as optional and default to "None", but that causes a runtime error. dni_extra refers to "Extraterrestrial direct normal irradiance"
Function: https://pvlib-python.readthedocs.io/en/stable/reference/generated/pvlib.irradiance.ghi_from_poa_driesse_2023.html

To Reproduce

import pvlib
import numpy as np
import pandas as pd

# Define the site and module parameters
site_lat = 41.5	# Site latitude
site_lon = -4.0	# Site longitude
site_tz = 'Etc/UTC'	# Site timezone

# Define the tracking system parameters
axis_tilt = 0		# Tilt of the tracker axis
axis_azimuth = 180	# Tracker axis facing south
max_angle = 75		# Max tilt angle for the tracker
gcr = 0.5			# Ground coverage ratio
backtrack = True	# Enable backtracking to avoid shading

# Create the location object
location = pvlib.location.Location(site_lat, site_lon, tz=site_tz)

# Run solar position calculation
times = pd.date_range('2024-01-01 00:00', '2024-01-10 00:00', freq='15min', tz=site_tz, inclusive='left')
solar_position = location.get_solarposition(times)

# Set up the tracking system with backtracking
tracking_data = pvlib.tracking.singleaxis(
      apparent_zenith=solar_position['apparent_zenith'],
      apparent_azimuth=solar_position['azimuth'],
      axis_tilt=axis_tilt,
      axis_azimuth=axis_azimuth,
      max_angle=max_angle,
      backtrack=backtrack,
      gcr=gcr
)

# GHI 2 POA. Estimate global horizontal irradiance (GHI) from global plane-of-array (POA) irradiance
#	https://pvlib-python.readthedocs.io/en/stable/reference/generated/pvlib.irradiance.ghi_from_poa_driesse_2023.html
solar_position['poa_global'] = 1000 * np.random.rand(len(solar_position))
solar_position['ghi'] = pvlib.irradiance.ghi_from_poa_driesse_2023(surface_tilt=tracking_data['surface_tilt'],
                                        surface_azimuth=tracking_data['surface_azimuth'],
                                        solar_zenith=solar_position['apparent_zenith'],
                                        solar_azimuth=solar_position['azimuth'],
                                        poa_global=solar_position['poa_global'])

Output of previous code

  File "/var/my_env/lib/python3.10/site-packages/pvlib/irradiance.py", line 1589, in ghi_from_poa_driesse_2023
    ghi, conv, niter = ghi_from_poa_array(surface_tilt, surface_azimuth,
  File "/var/my_env/lib/python3.10/site-packages/numpy/lib/function_base.py", line 2328, in __call__
    return self._vectorize_call(func=func, args=vargs)
  File "/var/my_env/lib/python3.10/site-packages/numpy/lib/function_base.py", line 2406, in _vectorize_call
    ufunc, otypes = self._get_ufunc_and_otypes(func=func, args=args)
  File "/var/my_env/lib/python3.10/site-packages/numpy/lib/function_base.py", line 2366, in _get_ufunc_and_otypes
    outputs = func(*inputs)
  File "/var/my_env/lib/python3.10/site-packages/numpy/lib/function_base.py", line 2323, in func
    return self.pyfunc(*the_args, **kwargs)
  File "/var/my_env/lib/python3.10/site-packages/pvlib/irradiance.py", line 1492, in _ghi_from_poa
    ghi_clear = dni_extra * tools.cosd(solar_zenith)
TypeError: unsupported operand type(s) for *: 'NoneType' and 'float

Expected behavior
No need to define "dni_extra", or do not define it as optional

Versions:

• pvlib.__version__: 0.11.1
• pandas.__version__: 1.5.2
• python: 3.10

[iblasi]

I found a solution looking to another documentation example where it creates the required variable: https://pvlib-python.readthedocs.io/en/stable/gallery/irradiance-transposition/plot_rtranpose_year.html

So the issue can be solved modifying the final code in the following way to add dni_extra variable.

# GHI 2 POA. Estimate global horizontal irradiance (GHI) from global plane-of-array (POA) irradiance
#	https://pvlib-python.readthedocs.io/en/stable/reference/generated/pvlib.irradiance.ghi_from_poa_driesse_2023.html
solar_position['poa_global'] = 1000 * np.random.rand(len(solar_position))
solar_position['dni_extra'] = pvlib.irradiance.get_extra_radiation(solar_position.index)
solar_position['ghi'] = pvlib.irradiance.ghi_from_poa_driesse_2023(surface_tilt=tracking_data['surface_tilt'],
                                        surface_azimuth=tracking_data['surface_azimuth'],
                                        solar_zenith=solar_position['apparent_zenith'],
                                        solar_azimuth=solar_position['azimuth'],
                                        poa_global=solar_position['poa_global'],
                                        dni_extra=solar_position['dni_extra'])

Still an issue
Although this solves the runtime error, the documentation should be updated and do not define it as optional, or if the optional parameter dni_extra is None, create that value with the get_extra_radiation function as described.

Note: This example has random poa_global values so some NaN might appear on the ghi result. I suppose tht it will be due to these random values, but I haven't checked it.

[cwhanse]

Confirmed. Every test of ghi_from_poa_driesse_2023 specifies dni_extra=1366.1, so we didn't catch this.

The private function _ghi_from_poa requires a value of dni_extra that can be multiplied by a float. I think setting dni_extra=1366.1 as the default could be the fix here.

[adriesse]

My bad! I am currently inclined to provide fewer defaults to make people think more about what they're doing. So another option would be to remove the default value here.

[markcampanelli]

I too fret about default values! One thought I’ve had is to make such parameters Optional[float] (so a float or None is expected), but do NOT give it a default value. In this way, the user must explicitly choose and pass None to “opt in” to using the default value. A downside here is that the (arguably) breaking change of changing the default value is more hidden because it no longer appears right in the function signature. Mark Campanelli LinkedIn <https://www.linkedin.com/in/markcampanelli/> Intelligent Measurement Systems LLC <https://intelligentmeasurementsystems.com> Try PVfit <https://pvfit.app> today!

…

On Wed, Oct 30, 2024 at 04:45 Anton Driesse ***@***.***> wrote: My bad! I am currently included to provide fewer defaults to make people think more about what they're doing. So another option would be to remove the default value here. — Reply to this email directly, view it on GitHub <#2279 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAISX42JYATUVPLRXQHHM2TZ6C2DZAVCNFSM6AAAAABQ2IYO7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBWGUZTCMRSG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[cwhanse]

So another option would be to remove the default value here.

Requiring dni_extra doesn't seem like a burden on the user. As the method's originator, which would you prefer? No default, or default to the solar constant?

[adriesse]

My preference is for no default value to be consistent with perez and perez_driesse.