Should we pre-calculate extra-radiation for pvlib.irradiance.get_total_irradiance(..., dni_extra=None, ...)

[Mark Mikofski]

Hi All,

This always trips me up. Sorry if I've asked or this has been answered before, but if I don't provide the extra_dni, and I use say like Hay-Davies, then I get a very confusing, cryptic stacktrace. I have to pre-calculate this first to pass to pvlib.irradiance.get_total_irradiance()

that seems fine for the underlying haydavies function but for the wrapper, seems like pvlib could just calculate this for me if I leave it as None.

Does anyone else agree or disagree or have any opinion?

thanks,

Mark

[Anton Driesse]

Opinion: cryptic stacktrace ==  'bad'

Anton

--
You received this message because you are subscribed to the Google Groups "pvlib-python" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pvlib-python...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/pvlib-python/4557b733-7526-4989-af43-e1680f059d54o%40googlegroups.com.

-- 
Photovoltaic Performance Labs Germany
Emmy-Noether-Str. 2
79110 Freiburg
Germany

+49-761-8973-5603 (Office)
+49-174-532-7677 (Mobile)

www.pvperformancelabs.com 

[kevina...@gmail.com]

I think something similar has come up previously: https://groups.google.com/g/pvlib-python/c/ZPMdpQOD6F4/m/ooEVC4H1BAAJ

I was of the opinion at the time that a _build_kwargs-like solution was a good idea -- checking the provided parameters against the requirements for the specified model and raising a more useful error if some are missing.  I don't feel too strongly either way about calculating defaults if a good error message is provided.

[cwh...@sandia.gov]

Can you post a code snippet? I've been thinking about ways to make PVSystem and ModelChain more fault tolerant, or more courteous when required values are not provided.

[kevina...@gmail.com]

I was imagining something like this modified get_sky_diffuse below.  Looking at it written out, it seems a little silly to have that much machinery when only two of the parameters (dni_extra and airmass) can be missing.  Might be better to just add special cases to the existing function like `if dni_extra is None and model == 'haydavies': raise ...`

As an aside, maybe it's too "magical", but we could use the inspect library to retrieve the function parameters automatically so that they don't have to be hardcoded in get_sky_diffuse:  inspect.signature(pvlib.irradiance.haydavies).parameters.keys()

def get_sky_diffuse(surface_tilt, surface_azimuth,
                    solar_zenith, solar_azimuth,
                    dni, ghi, dhi, dni_extra=None, airmass=None,
                    model='isotropic',
                    model_perez='allsitescomposite1990'):

    inputs = dict(surface_tilt=surface_tilt, surface_azimuth=surface_azimuth,
                  solar_zenith=solar_zenith, solar_azimuth=solar_azimuth,
                  dni=dni, ghi=ghi, dhi=dhi, dni_extra=dni_extra,
                  airmass=airmass, model=model, model_perez=model_perez)
    model = model.lower()

    if model == 'isotropic':
        keys = ['surface_tilt', 'dhi']
        model_func = isotropic
    elif model == 'klucher':
        keys = ['surface_tilt', 'surface_azimuth', 'dhi', 'ghi',
                'solar_zenith', 'solar_azimuth']
        model_func = klucher
    elif model == 'haydavies':
        ...
    else:
        raise ValueError('invalid model selection {}'.format(model))

    inputs = {k: v for k, v in inputs.items() if v is not None}
    kwargs = tools._build_kwargs(keys, inputs)
    sky = model_func(**kwargs)  # complains about missing arg if not present

    return sky

[kevina...@gmail.com]

It occurred to me that a slightly different but perhaps cleaner idea is to decorate the low-level functions to check them for None values no matter where they get called from.  Here's a prototype decorator:

def require_values(*parameter_names):
    """
    Check the function call to make sure the specified arguments are not None.
    """
    def factory(function):
        @functools.wraps(function)
        def wrapper(*args, **kwargs):
            # match up the argument values with the function parameter names
            signature = inspect.signature(function)
            # `binding` is now a dict of parameter name -> argument value
            binding = signature.bind(*args, **kwargs).arguments
            # find which of the specified arguments are none
            none_subset = [
                param for param in parameter_names if binding[param] is None
            ]
            if none_subset:
                funcname = function.__name__
                error_template = "Arguments to function `{}` cannot be None: "
                error_message = error_template.format(funcname)
                raise ValueError(error_message + ", ".join(none_subset))

            return function(*args, **kwargs)
        return wrapper
    return factory

Sorry for the big block of non-highlighted code.  It would be used to decorate functions and specify the parameters that cannot be None.  Some functions (e.g. irradiance.haydavies) have parameters that are allowed to be None, so the decorator can't just check all arguments.  More concretely: it would decorate the functions `irradiance.perez`, `irradiance.haydavies` and such so that if a user doesn't specify all the required arguments in `irradiance.get_total_irradiance`, they get a nicer error message.  Something like this:

@require_values('surface_tilt', 'surface_azimuth', 'dhi', 'dni', 'dni_extra')
def haydavies(surface_tilt, surface_azimuth, dhi, dni, dni_extra,
              solar_zenith=None, solar_azimuth=None, projection_ratio=None):

    ...

get_total_irradiance(20, 180, 45, 45, 100, 300, 200, model='haydavies')
Traceback (most recent call last):
...
ValueError: Arguments to function `haydavies` cannot be None: dni_extra

Does this seem like a good idea?  I can open a PR to show it in action if so.

Kevin

[William Holmgren]

I'd rather not introduce that kind of complexity to the lower level pvlib functions.

Ideas that I support:

* calculate if not provided (Mark's original idea)

* reraising a more informative message.

* require all of the arguments in get_sky_diffuse and get_total_irradiance regardless of whether or not they're used.

* remove the arguments from these functions and do the calculation if it's needed

* remove get_sky_diffuse and get_total_irradiance. I'm not convinced they're a net positive for the library.

Will

--

You received this message because you are subscribed to the Google Groups "pvlib-python" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pvlib-python...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/pvlib-python/93813715-bf4c-42e0-be26-d5644d0aaf61n%40googlegroups.com.