Skip to content

Make interp IAM method available for modelchain #1832

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Sep 8, 2023
Merged

Make interp IAM method available for modelchain #1832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ce2fb60
Make interp method available for modelchain
echedey-ls Aug 12, 2023
f562c28
Update test_pvsystem.py
echedey-ls Aug 12, 2023
ac7af3c
Update v0.10.2.rst
echedey-ls Aug 12, 2023
fc996ea
Apply Kevin's suggestions
echedey-ls Aug 15, 2023
8febd15
Update test_modelchain.py
echedey-ls Aug 15, 2023
9cc8e68
Merge branch 'main' into add-aoi-interp-to-modelchain
echedey-ls Aug 16, 2023
c5e0556
Update error message in modelchain.py
echedey-ls Aug 25, 2023
b079512
Allow non _IAM_MODEL_PARAMS to be passed to IAM models in pvsystem
echedey-ls Aug 26, 2023
efe35ee
Merge branch 'main' into add-aoi-interp-to-modelchain
echedey-ls Aug 26, 2023
458dbce
Add kwargs test
echedey-ls Aug 29, 2023
3db5704
Update v0.10.2.rst
echedey-ls Sep 6, 2023
f544ba4
Update pvlib/modelchain.py
echedey-ls Sep 6, 2023
510947c
Merge branch 'add-aoi-interp-to-modelchain' of https://github.com/ech…
echedey-ls Sep 6, 2023
4054c66
Update modelchain.py
echedey-ls Sep 6, 2023
da87b9f
Merge branch 'main' into add-aoi-interp-to-modelchain
echedey-ls Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/sphinx/source/whatsnew/v0.10.2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,17 @@ Enhancements
:py:func:`pvlib.clearsky.detect_clearsky` (:issue:`1808`, :pull:`1784`)
* Added a continuous version of the Erbs diffuse-fraction/decomposition model.
:py:func:`pvlib.irradiance.erbs_driesse` (:issue:`1755`, :pull:`1834`)

* Added :py:func:`~pvlib.iam.interp` option as AOI losses model in
:py:class:`pvlib.modelchain.ModelChain` and
:py:class:`pvlib.pvsystem.PVSystem`. (:issue:`1742`, :pull:`1832`)

Bug fixes
~~~~~~~~~
* :py:func:`~pvlib.iotools.get_psm3` no longer incorrectly returns clear-sky
DHI instead of clear-sky GHI when requesting ``ghi_clear``. (:pull:`1819`)
* :py:class:`pvlib.pvsystem.PVSystem` now correctly passes ``n_ar`` module
parameter to :py:func:`pvlib.iam.physical` when this IAM model is specified
or inferred. (:pull:`1832`)

Testing
~~~~~~~
Expand Down Expand Up @@ -53,6 +58,8 @@ Contributors
* Adam R. Jensen (:ghuser:`AdamRJensen`)
* Abigail Jones (:ghuser:`ajonesr`)
* Taos Transue (:ghuser:`reepoi`)
* Echedey Luis (:ghuser:`echedey-ls`)
* Todd Karin (:ghuser:`toddkarin`)
* NativeSci (:ghuser:`nativesci`)
* Anton Driesse (:ghuser:`adriesse`)
* Lukas Grossar (:ghuser:`tongpu`)
Expand Down
2 changes: 1 addition & 1 deletion pvlib/iam.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
'physical': {'n', 'K', 'L'},
'martin_ruiz': {'a_r'},
'sapm': {'B0', 'B1', 'B2', 'B3', 'B4', 'B5'},
'interp': set()
'interp': {'theta_ref', 'iam_ref'}
}


Expand Down
34 changes: 22 additions & 12 deletions pvlib/modelchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@
from typing import Union, Tuple, Optional, TypeVar

from pvlib import (atmosphere, clearsky, inverter, pvsystem, solarposition,
temperature)
temperature, iam)
import pvlib.irradiance # avoid name conflict with full import
from pvlib.pvsystem import _DC_MODEL_PARAMS
from pvlib._deprecation import pvlibDeprecationWarning
from pvlib.tools import _build_kwargs

from pvlib._deprecation import deprecated
Expand Down Expand Up @@ -279,7 +278,7 @@ def _mcr_repr(obj):
# scalar, None, other?
return repr(obj)


# Type for fields that vary between arrays
T = TypeVar('T')

Expand Down Expand Up @@ -490,7 +489,7 @@ class ModelChain:
If None, the model will be inferred from the parameters that
are common to all of system.arrays[i].module_parameters.
Valid strings are 'physical', 'ashrae', 'sapm', 'martin_ruiz',
'no_loss'. The ModelChain instance will be passed as the
'interp' and 'no_loss'. The ModelChain instance will be passed as the
first argument to a user-defined function.

spectral_model: None, str, or function, default None
Expand Down Expand Up @@ -917,6 +916,8 @@ def aoi_model(self, model):
self._aoi_model = self.sapm_aoi_loss
elif model == 'martin_ruiz':
self._aoi_model = self.martin_ruiz_aoi_loss
elif model == 'interp':
self._aoi_model = self.interp_aoi_loss
elif model == 'no_loss':
self._aoi_model = self.no_aoi_loss
else:
Expand All @@ -928,22 +929,24 @@ def infer_aoi_model(self):
module_parameters = tuple(
array.module_parameters for array in self.system.arrays)
params = _common_keys(module_parameters)
if {'K', 'L', 'n'} <= params:
if iam._IAM_MODEL_PARAMS['physical'] <= params:
return self.physical_aoi_loss
elif {'B5', 'B4', 'B3', 'B2', 'B1', 'B0'} <= params:
elif iam._IAM_MODEL_PARAMS['sapm'] <= params:
return self.sapm_aoi_loss
elif {'b'} <= params:
elif iam._IAM_MODEL_PARAMS['ashrae'] <= params:
return self.ashrae_aoi_loss
elif {'a_r'} <= params:
elif iam._IAM_MODEL_PARAMS['martin_ruiz'] <= params:
return self.martin_ruiz_aoi_loss
elif iam._IAM_MODEL_PARAMS['interp'] <= params:
return self.interp_aoi_loss
else:
raise ValueError('could not infer AOI model from '
'system.arrays[i].module_parameters. Check that '
'the module_parameters for all Arrays in '
'system.arrays contain parameters for '
'the physical, aoi, ashrae or martin_ruiz model; '
'explicitly set the model with the aoi_model '
'kwarg; or set aoi_model="no_loss".')
'system.arrays contain parameters for the '
'physical, aoi, ashrae, martin_ruiz or interp '
'model; explicitly set the model with the '
'aoi_model kwarg; or set aoi_model="no_loss".')

def ashrae_aoi_loss(self):
self.results.aoi_modifier = self.system.get_iam(
Expand Down Expand Up @@ -972,6 +975,13 @@ def martin_ruiz_aoi_loss(self):
)
return self

def interp_aoi_loss(self):
self.results.aoi_modifier = self.system.get_iam(
self.results.aoi,
iam_model='interp'
)
return self

def no_aoi_loss(self):
if self.system.num_arrays == 1:
self.results.aoi_modifier = 1.0
Expand Down
19 changes: 10 additions & 9 deletions pvlib/pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io
import itertools
import os
import inspect
from urllib.request import urlopen
import numpy as np
from scipy import constants
Expand Down Expand Up @@ -388,7 +389,7 @@ def get_iam(self, aoi, iam_model='physical'):

aoi_model : string, default 'physical'
The IAM model to be used. Valid strings are 'physical', 'ashrae',
'martin_ruiz' and 'sapm'.
'martin_ruiz', 'sapm' and 'interp'.
Returns
-------
iam : numeric or tuple of numeric
Expand Down Expand Up @@ -1151,7 +1152,7 @@ def get_iam(self, aoi, iam_model='physical'):

aoi_model : string, default 'physical'
The IAM model to be used. Valid strings are 'physical', 'ashrae',
'martin_ruiz' and 'sapm'.
'martin_ruiz', 'sapm' and 'interp'.

Returns
-------
Expand All @@ -1164,16 +1165,16 @@ def get_iam(self, aoi, iam_model='physical'):
if `iam_model` is not a valid model name.
"""
model = iam_model.lower()
if model in ['ashrae', 'physical', 'martin_ruiz']:
param_names = iam._IAM_MODEL_PARAMS[model]
kwargs = _build_kwargs(param_names, self.module_parameters)
func = getattr(iam, model)
if model in ['ashrae', 'physical', 'martin_ruiz', 'interp']:
func = getattr(iam, model) # get function at pvlib.iam
# get all parameters from function signature to retrieve them from
# module_parameters if present
params = set(inspect.signature(func).parameters.keys())
params.discard('aoi') # exclude aoi so it can't be repeated
kwargs = _build_kwargs(params, self.module_parameters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder if the IAM parameters should have been tracked in their own separate data structure instead of having to be extracted from module_parameters. (no action needed in this PR; just thinking out loud)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they only reason they were in module_parameters is because the old SAPM database includes them. That's not a reason to keep this as they are, in my view.

return func(aoi, **kwargs)
elif model == 'sapm':
return iam.sapm(aoi, self.module_parameters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice also that we don't have a way to pass upper through to pvlib.iam.sapm. Separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm. I don't like many things about the current interface of these IAM functions; IMO, a good approach is shown at physical:

def physical(aoi, n=1.526, K=4.0, L=0.002, *, n_ar=None):

Note the asterisk (it means following args are keyword-only args just in case).

Each argument, of different kinds, parse to a real-world meaning (again, IMO).

POSITIONAL_ONLY (before /) & POSITIONAL_OR_KEYWORD: used for required input to run models. In the case of AOI modifiers, it is the aoi parameter. We see that they can also be used to specify defaults for the model params. {aoi, n, K, L}

KEYWORD_ONLY: used to specify additional inputs, that are not required in the published paper of the model. {n_ar}

I would still upgrade the function signature to:

def physical(aoi, /, n=1.526, K=4.0, L=0.002, *, n_ar=None):

So it is always the first parameter the input. This is completely unnecessary in IAM models since the only required input to work is aoi.

Why this proposal?

Because of the inspect module. In this PR I used it to retrieve all the parameters needed:

if model in ['ashrae', 'physical', 'martin_ruiz', 'interp']:
    func = getattr(iam, model)  # get function at pvlib.iam
    # get all parameters from function signature to retrieve them from
    # module_parameters if present
    params = set(inspect.signature(func).parameters.keys())
    params.discard('aoi')  # exclude aoi so it can't be repeated
    kwargs = _build_kwargs(params, self.module_parameters)
    return func(aoi, **kwargs)

With this broad approach to the function parameter meanings, we could test for the types of each one and infer it's use with inspect: we could stop maintaining iam._IAM_MODEL_PARAMETERS, which only is used to infer the right model as stated by @cwhanse . Except for iam.sapm, its interface is special but could be changed easily.

I tried not to use iam._IAM_MODEL_PARAMETERS in the whole code, but with no luck OFC, functions interfaces had to be changed - a breaking API change.

In short,

I propose a normalized way of defining function signatures, where:

  • POSITIONAL_ONLY arguments are the required ones to get numerical values that may make sense (this is open to debate [1])
  • POSITIONAL_OR_KEYWORD are the ones that fit a function or a physical magnitude of interest, that usually (always?) has a default values
  • KEYWORD_ONLY args are completely optional to run the model
    So with the inspect module utilities all this information is easily obtained and no more inference dicts are needed.

[1] Since also required ones are those without default values and it can also be checked with the inspect module.

In the case of IAM module:

def ashrae(aoi, /, b=0.05):
def physical(aoi, /, n=1.526, K=4.0, L=0.002, *, n_ar=None):
def martin_ruiz(aoi, /, a_r=0.16):
def martin_ruiz_diffuse(surface_tilt, /, a_r=0.16, c1=0.4244, *, c2=None):
def interp(aoi, theta_ref, iam_ref, *, method='linear', normalize=True):
def sapm(aoi, /, module, *, upper=None):  # here module should be changed to B5 ... B0
...

Or without the /,, as stated per [1].

I see this got really long, may I open a discussion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may I open a discussion?

Absolutely! Better to have a dedicated thread for this. Otherwise it will get buried soon when the PR is merged :)

elif model == 'interp':
raise ValueError(model + ' is not implemented as an IAM model '
'option for Array')
else:
raise ValueError(model + ' is not a valid IAM model')

Expand Down
43 changes: 42 additions & 1 deletion pvlib/tests/test_modelchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1455,6 +1455,27 @@ def test_aoi_model_no_loss(sapm_dc_snl_ac_system, location, weather):
assert mc.results.ac[1] < 1


def test_aoi_model_interp(sapm_dc_snl_ac_system, location, weather, mocker):
# similar to test_aoi_models but requires arguments to work, so we
# add 'interp' aoi losses model arguments to module
iam_ref = (1., 0.85)
theta_ref = (0., 80.)
sapm_dc_snl_ac_system.arrays[0].module_parameters['iam_ref'] = iam_ref
sapm_dc_snl_ac_system.arrays[0].module_parameters['theta_ref'] = theta_ref
mc = ModelChain(sapm_dc_snl_ac_system, location,
dc_model='sapm', aoi_model='interp',
spectral_model='no_loss')
m = mocker.spy(iam, 'interp')
mc.run_model(weather=weather)
# only test kwargs
assert m.call_args[1]['iam_ref'] == iam_ref
assert m.call_args[1]['theta_ref'] == theta_ref
assert isinstance(mc.results.ac, pd.Series)
assert not mc.results.ac.empty
assert mc.results.ac[0] > 150 and mc.results.ac[0] < 200
assert mc.results.ac[1] < 1


def test_aoi_model_user_func(sapm_dc_snl_ac_system, location, weather, mocker):
m = mocker.spy(sys.modules[__name__], 'constant_aoi_loss')
mc = ModelChain(sapm_dc_snl_ac_system, location, dc_model='sapm',
Expand All @@ -1468,7 +1489,7 @@ def test_aoi_model_user_func(sapm_dc_snl_ac_system, location, weather, mocker):


@pytest.mark.parametrize('aoi_model', [
'sapm', 'ashrae', 'physical', 'martin_ruiz'
'sapm', 'ashrae', 'physical', 'martin_ruiz', 'interp'
])
def test_infer_aoi_model(location, system_no_aoi, aoi_model):
for k in iam._IAM_MODEL_PARAMS[aoi_model]:
Expand All @@ -1477,6 +1498,26 @@ def test_infer_aoi_model(location, system_no_aoi, aoi_model):
assert isinstance(mc, ModelChain)


@pytest.mark.parametrize('aoi_model,model_kwargs', [
# model_kwargs has both required and optional kwargs; test all
('physical',
{'n': 1.526, 'K': 4.0, 'L': 0.002, # required
'n_ar': 1.8}), # extra
('interp',
{'theta_ref': (0, 75, 85, 90), 'iam_ref': (1, 0.8, 0.42, 0), # required
'method': 'cubic', 'normalize': False})]) # extra
def test_infer_aoi_model_with_extra_params(location, system_no_aoi, aoi_model,
model_kwargs, weather, mocker):
# test extra parameters not defined at iam._IAM_MODEL_PARAMS are passed
m = mocker.spy(iam, aoi_model)
system_no_aoi.arrays[0].module_parameters.update(**model_kwargs)
mc = ModelChain(system_no_aoi, location, spectral_model='no_loss')
assert isinstance(mc, ModelChain)
mc.run_model(weather=weather)
_, call_kwargs = m.call_args
assert call_kwargs == model_kwargs


def test_infer_aoi_model_invalid(location, system_no_aoi):
exc_text = 'could not infer AOI model'
with pytest.raises(ValueError, match=exc_text):
Expand Down
13 changes: 9 additions & 4 deletions pvlib/tests/test_pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,15 @@ def test_PVSystem_get_iam_sapm(sapm_module_params, mocker):
assert_allclose(out, 1.0, atol=0.01)


def test_PVSystem_get_iam_interp(sapm_module_params, mocker):
system = pvsystem.PVSystem(module_parameters=sapm_module_params)
with pytest.raises(ValueError):
system.get_iam(45, iam_model='interp')
def test_PVSystem_get_iam_interp(mocker):
interp_module_params = {'iam_ref': (1., 0.8), 'theta_ref': (0., 80.)}
system = pvsystem.PVSystem(module_parameters=interp_module_params)
spy = mocker.spy(_iam, 'interp')
aoi = ((0., 40., 80.),)
expected = (1., 0.9, 0.8)
out = system.get_iam(aoi, iam_model='interp')
assert_allclose(out, expected)
spy.assert_called_once_with(aoi[0], **interp_module_params)


def test__normalize_sam_product_names():
Expand Down