Skip to content

Add desoto and pvsyst as dc_model options to ModelChain #555

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.

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
Sep 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 5 additions & 4 deletions docs/sphinx/source/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,10 @@ AOI modifiers
pvsystem.ashraeiam
pvsystem.sapm_aoi_loss

Single diode model
------------------
Single diode models
-------------------

Functions relevant for the single diode model.
Functions relevant for single diode models.

.. autosummary::
:toctree: generated/
Expand Down Expand Up @@ -419,7 +419,8 @@ ModelChain model definitions.
:toctree: generated/

modelchain.ModelChain.sapm
modelchain.ModelChain.singlediode
modelchain.ModelChain.desoto
modelchain.ModelChain.pvsyst
modelchain.ModelChain.pvwatts_dc
modelchain.ModelChain.snlinverter
modelchain.ModelChain.adrinverter
Expand Down
3 changes: 3 additions & 0 deletions docs/sphinx/source/whatsnew/v0.6.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ API Changes
dirindex functions. (:issue:`311`, :issue:`396`)
* Method ModelChain.infer_dc_model now returns a tuple (function handle, model name string)
instead of only the function handle (:issue:`417`)
* Add DC model methods desoto and pvsyst to ModelChain, and deprecates DC model method singlediode
(singlediode defaults to desoto until v0.7.0) (:issue:`487`)


Enhancements
Expand Down Expand Up @@ -104,6 +106,7 @@ Enhancements
* Add irradiance.clearness_index function. (:issue:`396`)
* Add irradiance.clearness_index_zenith_independent function. (:issue:`396`)
* Add checking for consistency between module_parameters and dc_model. (:issue:`417`)
* Add DC model methods desoto and pvsyst to ModelChain (:issue:`487`)


Bug fixes
Expand Down
60 changes: 55 additions & 5 deletions pvlib/modelchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from pvlib.tracking import SingleAxisTracker
import pvlib.irradiance # avoid name conflict with full import
from pvlib.pvsystem import DC_MODEL_PARAMS
from pvlib._deprecation import pvlibDeprecationWarning


def basic_chain(times, latitude, longitude,
Expand Down Expand Up @@ -251,8 +252,8 @@ class ModelChain(object):
dc_model: None, str, or function, default None
If None, the model will be inferred from the contents of
system.module_parameters. Valid strings are 'sapm',
'singlediode', 'pvwatts'. The ModelChain instance will be passed
as the first argument to a user-defined function.
'desoto', 'pvsyst', 'pvwatts'. The ModelChain instance will
be passed as the first argument to a user-defined function.

ac_model: None, str, or function, default None
If None, the model will be inferred from the contents of
Expand Down Expand Up @@ -379,10 +380,19 @@ def dc_model(self, model):
str(missing_params))
if model == 'sapm':
self._dc_model = self.sapm
elif model == 'singlediode':
self._dc_model = self.singlediode
elif model == 'desoto':
self._dc_model = self.desoto
elif model == 'pvsyst':
self._dc_model = self.pvsyst
elif model == 'pvwatts':
self._dc_model = self.pvwatts_dc
elif model == 'singlediode':
warnings.warn('DC model keyword singlediode used for '
'ModelChain object. singlediode is '
'ambiguous, use desoto instead. singlediode '
'keyword will be removed in v0.7.0 and '
'later', pvlibDeprecationWarning)
self._dc_model = self.desoto
else:
raise ValueError(model + ' is not a valid DC power model')
else:
Expand All @@ -393,7 +403,10 @@ def infer_dc_model(self):
if set(['A0', 'A1', 'C7']) <= params:
return self.sapm, 'sapm'
elif set(['a_ref', 'I_L_ref', 'I_o_ref', 'R_sh_ref', 'R_s']) <= params:
return self.singlediode, 'singlediode'
return self.desoto, 'desoto'
elif set(['gamma_ref', 'mu_gamma', 'I_L_ref', 'I_o_ref',
'R_sh_ref', 'R_sh_0', 'R_sh_exp', 'R_s']) <= params:
return self.pvsyst, 'pvsyst'
elif set(['pdc0', 'gamma_pdc']) <= params:
return self.pvwatts_dc, 'pvwatts'
else:
Expand All @@ -408,7 +421,44 @@ def sapm(self):

return self

def desoto(self):
(photocurrent, saturation_current, resistance_series,
resistance_shunt, nNsVth) = (
self.system.calcparams_desoto(self.effective_irradiance,
self.temps['temp_cell']))

self.diode_params = (photocurrent, saturation_current,
resistance_series,
resistance_shunt, nNsVth)

self.dc = self.system.singlediode(
photocurrent, saturation_current, resistance_series,
resistance_shunt, nNsVth)

self.dc = self.system.scale_voltage_current_power(self.dc).fillna(0)

return self

def pvsyst(self):
(photocurrent, saturation_current, resistance_series,
resistance_shunt, nNsVth) = (
self.system.calcparams_pvsyst(self.effective_irradiance,
self.temps['temp_cell']))

self.diode_params = (photocurrent, saturation_current,
resistance_series,
resistance_shunt, nNsVth)

self.dc = self.system.singlediode(
photocurrent, saturation_current, resistance_series,
resistance_shunt, nNsVth)

self.dc = self.system.scale_voltage_current_power(self.dc).fillna(0)

return self
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the very similar copy/paste code. But it's explicit and that is important too. So I lean towards this approach.

Copy link
Member

@mikofski mikofski Sep 5, 2018

Choose a reason for hiding this comment

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

I would mark this with FIXME. IMO the copy/paste only seems explicit, but it is WET code. DRY code is robust and not prone to errors. I agree this is probably maintainable in the short term, but I think it might become a burden in the future, especially if more "models" are added, such as pvsyst_v7, ...

There are only two lines of difference between these:

--- desoto.py
+++ pvsyst.py
@@ -1,7 +1,7 @@
-def desoto(self):
+def pvsyst(self):
     (photocurrent, saturation_current, resistance_series,
      resistance_shunt, nNsVth) = (
-        self.system.calcparams_desoto(self.effective_irradiance,
+        self.system.calcparams_pvsyst(self.effective_irradiance,
                                       self.temps['temp_cell']))
 
     self.diode_params = (photocurrent, saturation_current,

You could add a tiny dispatcher that you delegate this common task to that is DRY and IMHO also explicit:

def desoto(self):
    """calculate desoto model"""
    return self._model_dispatcher(self.system.calcparams_desoto, 'desoto')

def pvsyst(self):
    """calculate pvsyst model"""
    return self._model_dispatcher(self.system.calcparams_pvsyst, 'pvsyst')

def _model_dispatcher(self, model_calcparams, model_name=None):
    """
    Delegate that dispatches the model

    Parameters
    ----------
    model_calcparams : function
        A function that calculates the models parameters given irradiance and cell temperature.
    model_name : str
        Optional string that may be used to further distinguish the model in the future.
    """
    (photocurrent, saturation_current, resistance_series,
     resistance_shunt, nNsVth) = (
        model_calcparams(self.effective_irradiance,
                         self.temps['temp_cell']))

    self.diode_params = (photocurrent, saturation_current,
                         resistance_series,
                         resistance_shunt, nNsVth)

    self.dc = self.system.singlediode(
        photocurrent, saturation_current, resistance_series,
        resistance_shunt, nNsVth)

    self.dc = self.system.scale_voltage_current_power(self.dc).fillna(0)

    return self

Copy link
Member

Choose a reason for hiding this comment

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

Either approach is ok with me at this point. Keep in mind that we've had discussions elsewhere about whether or not it even makes sense to define these model-specific methods on ModelChain and if we could replace them with wrappers on PVSystem. So I'm not too concerned about perfection here.

Copy link
Member

Choose a reason for hiding this comment

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

okay, me too, I fine either way 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

@mikofski that's a good suggestion. You are right that we have more diode models waiting - CEC hasn't been connected up yet. But we will (at least I hope) also have system.doublediode someday, and perhaps other variations on the calculate parameters -> calculate IV curve pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep the bite small, merge this, and take up improvements in another pull request.


def singlediode(self):
"""Deprecated"""
(photocurrent, saturation_current, resistance_series,
resistance_shunt, nNsVth) = (
self.system.calcparams_desoto(self.effective_irradiance,
Expand Down
7 changes: 7 additions & 0 deletions pvlib/pvsystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@
'C7', 'Isco', 'Impo', 'Aisc', 'Aimp', 'Bvoco',
'Mbvoc', 'Bvmpo', 'Mbvmp', 'N', 'Cells_in_Series',
'IXO', 'IXXO', 'FD']),
'desoto' :
set(['alpha_sc', 'a_ref', 'I_L_ref', 'I_o_ref',
'R_sh_ref', 'R_s']),
'pvsyst' :
set(['gamma_ref', 'mu_gamma', 'I_L_ref', 'I_o_ref',
'R_sh_ref', 'R_sh_0', 'R_s', 'alpha_sc', 'EgRef',
'cells_in_series']),
'singlediode' :
set(['alpha_sc', 'a_ref', 'I_L_ref', 'I_o_ref',
'R_sh_ref', 'R_s']),
Expand Down
47 changes: 41 additions & 6 deletions pvlib/test/test_modelchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,13 @@
from pvlib.pvsystem import PVSystem
from pvlib.tracking import SingleAxisTracker
from pvlib.location import Location
from pvlib._deprecation import pvlibDeprecationWarning

from pandas.util.testing import assert_series_equal, assert_frame_equal
import pytest

from test_pvsystem import sam_data
from conftest import requires_scipy
from test_pvsystem import sam_data, pvsyst_module_params
from conftest import fail_on_pvlib_version, requires_scipy


@pytest.fixture
Expand Down Expand Up @@ -53,6 +54,20 @@ def cec_dc_snl_ac_system(sam_data):
return system


@pytest.fixture
def pvsyst_dc_snl_ac_system(sam_data):
module = 'PVsyst test module'
module_parameters = pvsyst_module_params()
module_parameters['b'] = 0.05
inverters = sam_data['cecinverter']
inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()
system = PVSystem(surface_tilt=32.2, surface_azimuth=180,
module=module,
module_parameters=module_parameters,
inverter_parameters=inverter)
return system


@pytest.fixture
def cec_dc_adr_ac_system(sam_data):
modules = sam_data['cecmod']
Expand Down Expand Up @@ -203,14 +218,26 @@ def poadc(mc):


@pytest.mark.parametrize('dc_model', [
'sapm', pytest.param('singlediode', marks=requires_scipy), 'pvwatts_dc'])
def test_infer_dc_model(system, cec_dc_snl_ac_system,
'sapm',
pytest.param('desoto', marks=requires_scipy),
pytest.param('pvsyst', marks=requires_scipy),
pytest.param('singlediode', marks=requires_scipy),
'pvwatts_dc'])
def test_infer_dc_model(system, cec_dc_snl_ac_system, pvsyst_dc_snl_ac_system,
pvwatts_dc_pvwatts_ac_system, location, dc_model,
weather, mocker):
dc_systems = {'sapm': system, 'singlediode': cec_dc_snl_ac_system,
dc_systems = {'sapm': system,
'desoto': cec_dc_snl_ac_system,
'pvsyst': pvsyst_dc_snl_ac_system,
'singlediode': cec_dc_snl_ac_system,
'pvwatts_dc': pvwatts_dc_pvwatts_ac_system}
dc_model_function = {'sapm': 'sapm',
'desoto': 'calcparams_desoto',
'pvsyst': 'calcparams_pvsyst',
'singlediode': 'calcparams_desoto',
'pvwatts_dc': 'pvwatts_dc'}
system = dc_systems[dc_model]
m = mocker.spy(system, dc_model)
m = mocker.spy(system, dc_model_function[dc_model])
mc = ModelChain(system, location,
aoi_model='no_loss', spectral_model='no_loss')
mc.run_model(weather.index, weather=weather)
Expand Down Expand Up @@ -410,6 +437,14 @@ def test_bad_get_orientation():
modelchain.get_orientation('bad value')


@fail_on_pvlib_version('0.7')
def test_deprecated_07():
with pytest.warns(pvlibDeprecationWarning):
mc = ModelChain(cec_dc_snl_ac_system, location,
dc_model='singlediode', # this should fail after 0.7
aoi_model='no_loss', spectral_model='no_loss')


@requires_scipy
def test_basic_chain_required(sam_data):
times = pd.DatetimeIndex(start='20160101 1200-0700',
Expand Down