Skip to content

refactor ModelChain inverter methods to use PVSystem.get_ac #1150

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 13 commits into from
Jan 28, 2021
Merged

refactor ModelChain inverter methods to use PVSystem.get_ac #1150

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8eb7ed5
deprecate methods
wholmgren Jan 27, 2021
2736f8b
remove new but unreleased methods
wholmgren Jan 27, 2021
457689a
refactor modelchain inverter methods for get_ac
wholmgren Jan 27, 2021
ec4dba7
update whats new
wholmgren Jan 27, 2021
90a268d
update api.rst
wholmgren Jan 27, 2021
ab4ab6e
remove tests for methods that no longer exist
wholmgren Jan 27, 2021
3e5aaa4
fix get_ac with sandia tests
wholmgren Jan 27, 2021
b603a29
fix get_ac with sandia tests
wholmgren Jan 27, 2021
02492e4
clean up pvsystem imports
wholmgren Jan 27, 2021
84ce209
add test of infer_ac_model
wholmgren Jan 27, 2021
b40c6ac
no v in deprecated args
wholmgren Jan 27, 2021
e7f4be7
fix pull number....
wholmgren Jan 27, 2021
7b8bb4e
remove _infer_ac_model_multi
wholmgren Jan 28, 2021
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
4 changes: 2 additions & 2 deletions docs/sphinx/source/api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,8 +630,8 @@ ModelChain model definitions.
modelchain.ModelChain.desoto
modelchain.ModelChain.pvsyst
modelchain.ModelChain.pvwatts_dc
modelchain.ModelChain.snlinverter
modelchain.ModelChain.adrinverter
modelchain.ModelChain.sandia_inverter
modelchain.ModelChain.adr_inverter
modelchain.ModelChain.pvwatts_inverter
modelchain.ModelChain.ashrae_aoi_loss
modelchain.ModelChain.physical_aoi_loss
Expand Down
14 changes: 9 additions & 5 deletions docs/sphinx/source/whatsnew/v0.9.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ Breaking changes
* ``irradiance.liujordan`` and ``ForecastModel.cloud_cover_to_irradiance_liujordan``
have been removed. (:pull:`1136`)

* ``ModelChain.snlinverter`` changed to ``ModelChain.sandia_inverter``.
``ModelChain.adrinverter`` changed to ``ModelChain.adr_inverter``.
(:pull:`1150`)


Deprecations
~~~~~~~~~~~~
Expand Down Expand Up @@ -76,9 +80,9 @@ Enhancements
* Support for :py:func:`~pvlib.inverter.sandia_multi` and
:py:func:`~pvlib.inverter.pvwatts_multi` added to
:py:class:`~pvlib.pvsystem.PVSystem` and
:py:class:`~pvlib.modelchain.ModelChain` (as ``ac_model='sandia_multi'``
and ``ac_model='pvwatts_multi'``).
(:pull:`1076`, :issue:`1067`, :pull:`1132`, :issue:`1117`)
:py:class:`~pvlib.modelchain.ModelChain` (as ``ac_model='sandia'``
and ``ac_model='pvwatts'``).
(:pull:`1076`, :issue:`1067`, :pull:`1132`, :issue:`1117`, :pull:`1150`)
* :py:class:`~pvlib.modelchain.ModelChain` 'run_model' methods now
automatically switch to using ``'effective_irradiance'`` (if available) for
cell temperature models, when ``'poa_global'`` is not provided in input
Expand All @@ -88,8 +92,8 @@ Enhancements
``pvsystem.PVSystem.strings_per_inverter``. Note that both attributes still
default to 1. (:pull:`1138`)
* :py:meth:`~pvlib.pvsystem.PVSystem.get_ac` is added to calculate AC power
from DC power. Use parameter 'model' to specify which inverter model to use.
(:pull:`1147`, :issue:`998`)
from DC power. Use parameter ``model`` to specify which inverter model to use.
(:pull:`1147`, :issue:`998`, :pull:`1150`)

Bug fixes
~~~~~~~~~
Expand Down
46 changes: 19 additions & 27 deletions pvlib/modelchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,15 +770,11 @@ def ac_model(self, model):
elif isinstance(model, str):
model = model.lower()
if model == 'sandia':
self._ac_model = self.snlinverter
elif model == 'sandia_multi':
self._ac_model = self.sandia_multi_inverter
self._ac_model = self.sandia_inverter
elif model in 'adr':
self._ac_model = self.adrinverter
self._ac_model = self.adr_inverter
elif model == 'pvwatts':
self._ac_model = self.pvwatts_inverter
elif model == 'pvwatts_multi':
self._ac_model = self.pvwatts_multi_inverter
else:
raise ValueError(model + ' is not a valid AC power model')
else:
Expand All @@ -790,9 +786,9 @@ def infer_ac_model(self):
if self.system.num_arrays > 1:
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need both infer_ac_model and infer_ac_model_multi? Maybe not.

Copy link
Member Author

Choose a reason for hiding this comment

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

The alternative is something like the nested if statements of PVSystem.get_ac. We need some way of raising a ValueError if num_arrays > 1 and the parameters are not consistent with pvwatts or sandia (essentially what we have now in ModelChain). Equivalently, for now, we could raise a ValueError if num_arrays > 1 and the parameters are consistent with adr (essentially what we have now in PVSystem.get_ac). This behavior is tested in test_ModelChain_invalid_inverter_params_arrays. I have a slight preference to leave it alone, but I'll change it if you prefer it more like PVSystem.get_ac.

Copy link
Member

@cwhanse cwhanse Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We need some way of raising a ValueError if num_arrays > 1 and the parameters are not consistent with pvwatts or sandia (essentially what we have now in ModelChain).

Agree. It might make more sense to do the validation in PVSystem.get_ac so that it catches both ModelChain and PVSystem users.

The _infer methods are only used when the inverter model isn't named, and the parameters for either _multi inverter model are the same as for the single MPPT version. That's why I don't see a need to keep both _infer methods since the ModelChain methods are aligned with the three model names rather than with the five inverter functions.

Assuming you agree with either of these points, I'm OK deferring to later work.

Copy link
Member Author

@wholmgren wholmgren Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

One more trade off here... The validation in the infer method helps avoid errors in PVSystem.get_ac during run_model. Traditionally ModelChain.run_model is nearly guaranteed to run if ModelChain can infer all methods at construction. Of course, as you point out, it only works if the user does not pass ac_model.

I'm generally -1 on input validation within pvlib so I'm ok with changing this in the long run.

Copy link
Member

Choose a reason for hiding this comment

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

Raise ValueError in _infer_ac_model when parameters indicate 'adr' and num_arrays > 1. Otherwise get_ac should be fine.

return self._infer_ac_model_multi(inverter_params)
if _snl_params(inverter_params):
return self.snlinverter
return self.sandia_inverter
if _adr_params(inverter_params):
return self.adrinverter
return self.adr_inverter
if _pvwatts_params(inverter_params):
return self.pvwatts_inverter
raise ValueError('could not infer AC model from '
Expand All @@ -802,38 +798,34 @@ def infer_ac_model(self):

def _infer_ac_model_multi(self, inverter_params):
if _snl_params(inverter_params):
return self.sandia_multi_inverter
return self.sandia_inverter
elif _pvwatts_params(inverter_params):
return self.pvwatts_multi_inverter
return self.pvwatts_inverter
raise ValueError('could not infer multi-array AC model from '
'system.inverter_parameters. Only sandia and pvwatts '
'inverter models support multiple '
'Arrays. Check system.inverter_parameters or '
'explicitly set the model with the ac_model kwarg.')

def sandia_multi_inverter(self):
self.results.ac = self.system.sandia_multi(
_tuple_from_dfs(self.results.dc, 'v_mp'),
_tuple_from_dfs(self.results.dc, 'p_mp')
def sandia_inverter(self):
self.results.ac = self.system.get_ac(
'sandia',
_tuple_from_dfs(self.results.dc, 'p_mp'),
v_dc=_tuple_from_dfs(self.results.dc, 'v_mp')
)
return self

def pvwatts_multi_inverter(self):
self.results.ac = self.system.pvwatts_multi(self.results.dc)
return self

def snlinverter(self):
self.results.ac = self.system.snlinverter(self.results.dc['v_mp'],
self.results.dc['p_mp'])
return self

def adrinverter(self):
self.results.ac = self.system.adrinverter(self.results.dc['v_mp'],
self.results.dc['p_mp'])
def adr_inverter(self):
self.results.ac = self.system.get_ac(
'adr',
self.results.dc['p_mp'],
v_dc=self.results.dc['v_mp']
)
return self

def pvwatts_inverter(self):
self.results.ac = self.system.pvwatts_ac(self.results.dc).fillna(0)
ac = self.system.get_ac('pvwatts', self.results.dc)
self.results.ac = ac.fillna(0)
return self

@property
Expand Down
32 changes: 3 additions & 29 deletions pvlib/pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import io
import os
from urllib.request import urlopen
import warnings
import numpy as np
import pandas as pd

Expand All @@ -17,7 +16,6 @@
from pvlib import (atmosphere, iam, inverter, irradiance,
singlediode as _singlediode, temperature)
from pvlib.tools import _build_kwargs
from pvlib._deprecation import pvlibDeprecationWarning


# a dict of required parameter names for each DC power model
Expand Down Expand Up @@ -921,6 +919,7 @@ def get_ac(self, model, p_dc, v_dc=None):
model + ' is not a valid AC power model.',
' model must be one of "sandia", "adr" or "pvwatts"')

@deprecated('0.9', alternative='PVSystem.get_ac', removal='0.10')
def snlinverter(self, v_dc, p_dc):
"""Uses :py:func:`pvlib.inverter.sandia` to calculate AC power based on
``self.inverter_parameters`` and the input voltage and power.
Expand All @@ -929,19 +928,7 @@ def snlinverter(self, v_dc, p_dc):
"""
return inverter.sandia(v_dc, p_dc, self.inverter_parameters)

def sandia_multi(self, v_dc, p_dc):
"""Uses :py:func:`pvlib.inverter.sandia_multi` to calculate AC power
based on ``self.inverter_parameters`` and the input voltage and power.

The parameters `v_dc` and `p_dc` must be tuples with length equal to
``self.num_arrays`` if the system has more than one array.

See :py:func:`pvlib.inverter.sandia_multi` for details.
"""
v_dc = self._validate_per_array(v_dc)
p_dc = self._validate_per_array(p_dc)
return inverter.sandia_multi(v_dc, p_dc, self.inverter_parameters)

@deprecated('0.9', alternative='PVSystem.get_ac', removal='0.10')
def adrinverter(self, v_dc, p_dc):
"""Uses :py:func:`pvlib.inverter.adr` to calculate AC power based on
``self.inverter_parameters`` and the input voltage and power.
Expand Down Expand Up @@ -1009,6 +996,7 @@ def pvwatts_losses(self):
self.losses_parameters)
return pvwatts_losses(**kwargs)

@deprecated('0.9', alternative='PVSystem.get_ac', removal='0.10')
def pvwatts_ac(self, pdc):
"""
Calculates AC power according to the PVWatts model using
Expand All @@ -1023,20 +1011,6 @@ def pvwatts_ac(self, pdc):
return inverter.pvwatts(pdc, self.inverter_parameters['pdc0'],
**kwargs)

def pvwatts_multi(self, p_dc):
"""Uses :py:func:`pvlib.inverter.pvwatts_multi` to calculate AC power
based on ``self.inverter_parameters`` and the input voltage and power.

The parameter `p_dc` must be a tuple with length equal to
``self.num_arrays`` if the system has more than one array.

See :py:func:`pvlib.inverter.pvwatts_multi` for details.
"""
p_dc = self._validate_per_array(p_dc)
kwargs = _build_kwargs(['eta_inv_nom', 'eta_inv_ref'],
self.inverter_parameters)
return inverter.pvwatts_multi(p_dc, self.inverter_parameters['pdc0'],
**kwargs)
@property
@_unwrap_single_value
def module_parameters(self):
Expand Down
51 changes: 30 additions & 21 deletions pvlib/tests/test_modelchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import numpy as np
import pandas as pd

from pvlib import iam, modelchain, pvsystem, temperature
from pvlib import iam, modelchain, pvsystem, temperature, inverter
from pvlib.modelchain import ModelChain
from pvlib.pvsystem import PVSystem
from pvlib.tracking import SingleAxisTracker
Expand Down Expand Up @@ -1235,27 +1235,36 @@ def acdc(mc):
mc.results.ac = mc.results.dc


@pytest.mark.parametrize('ac_model', ['sandia', 'adr',
'pvwatts', 'sandia_multi',
'pvwatts_multi'])
@pytest.mark.parametrize('inverter_model', ['sandia', 'adr',
'pvwatts', 'sandia_multi',
'pvwatts_multi'])
def test_ac_models(sapm_dc_snl_ac_system, cec_dc_adr_ac_system,
pvwatts_dc_pvwatts_ac_system, location, ac_model,
weather, mocker):
pvwatts_dc_pvwatts_ac_system, cec_dc_snl_ac_arrays,
pvwatts_dc_pvwatts_ac_system_arrays,
location, inverter_model, weather, mocker):
ac_systems = {'sandia': sapm_dc_snl_ac_system,
'sandia_multi': sapm_dc_snl_ac_system,
'sandia_multi': cec_dc_snl_ac_arrays,
'adr': cec_dc_adr_ac_system,
'pvwatts': pvwatts_dc_pvwatts_ac_system,
'pvwatts_multi': pvwatts_dc_pvwatts_ac_system}
ac_method_name = {'sandia': 'snlinverter',
'sandia_multi': 'sandia_multi',
'adr': 'adrinverter',
'pvwatts': 'pvwatts_ac',
'pvwatts_multi': 'pvwatts_multi'}
system = ac_systems[ac_model]

'pvwatts_multi': pvwatts_dc_pvwatts_ac_system_arrays}
inverter_to_ac_model = {
'sandia': 'sandia',
'sandia_multi': 'sandia',
'adr': 'adr',
'pvwatts': 'pvwatts',
'pvwatts_multi': 'pvwatts'}
ac_model = inverter_to_ac_model[inverter_model]
system = ac_systems[inverter_model]

mc_inferred = ModelChain(system, location,
aoi_model='no_loss', spectral_model='no_loss')
mc = ModelChain(system, location, ac_model=ac_model,
aoi_model='no_loss', spectral_model='no_loss')
m = mocker.spy(system, ac_method_name[ac_model])

# tests ModelChain.infer_ac_model
assert mc_inferred.ac_model.__name__ == mc.ac_model.__name__

m = mocker.spy(inverter, inverter_model)
mc.run_model(weather)
assert m.call_count == 1
assert isinstance(mc.results.ac, pd.Series)
Expand Down Expand Up @@ -1447,7 +1456,7 @@ def test_losses_models_no_loss(pvwatts_dc_pvwatts_ac_system, location, weather,

def test_invalid_dc_model_params(sapm_dc_snl_ac_system, cec_dc_snl_ac_system,
pvwatts_dc_pvwatts_ac_system, location):
kwargs = {'dc_model': 'sapm', 'ac_model': 'snlinverter',
kwargs = {'dc_model': 'sapm', 'ac_model': 'sandia',
'aoi_model': 'no_loss', 'spectral_model': 'no_loss',
'temperature_model': 'sapm', 'losses_model': 'no_loss'}
sapm_dc_snl_ac_system.module_parameters.pop('A0') # remove a parameter
Expand Down Expand Up @@ -1488,9 +1497,9 @@ def test_bad_get_orientation():
def test_with_sapm_pvsystem_arrays(sapm_dc_snl_ac_system_Array, location,
weather):
mc = ModelChain.with_sapm(sapm_dc_snl_ac_system_Array, location,
ac_model='sandia_multi')
ac_model='sandia')
assert mc.dc_model == mc.sapm
assert mc.ac_model == mc.sandia_multi_inverter
assert mc.ac_model == mc.sandia_inverter
mc.run_model(weather)
assert mc.results

Expand Down Expand Up @@ -1625,7 +1634,7 @@ def test_ModelChain___repr__(sapm_dc_snl_ac_system, location, strategy,
' solar_position_method: nrel_numpy',
' airmass_model: kastenyoung1989',
' dc_model: sapm',
' ac_model: snlinverter',
' ac_model: sandia_inverter',
' aoi_model: sapm_aoi_loss',
' spectral_model: sapm_spectral_loss',
' temperature_model: sapm_temp',
Expand Down Expand Up @@ -1778,7 +1787,7 @@ def test_inconsistent_array_params(location,
)
with pytest.raises(ValueError, match=temperature_error):
ModelChain(different_temp_system, location,
ac_model='sandia_multi',
ac_model='sandia',
aoi_model='no_loss', spectral_model='no_loss',
temperature_model='sapm')

Expand Down
Loading