Skip to content

add consistency tests to ModelChain.dc_model #548

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
Aug 31, 2018
Merged

add consistency tests to ModelChain.dc_model #548

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d3c1ae3
add consistency tests to ModelChain.dc_model
cwhanse Aug 27, 2018
6884555
Add self argument
cwhanse Aug 27, 2018
1c4ac04
Move DC_MODEL_PARAMS to pvsystem, streamline validation
cwhanse Aug 28, 2018
a04e111
Fix key for DC_MODEL_PARAMS
cwhanse Aug 28, 2018
3e1116b
Fix typo in IXO, IXXO
cwhanse Aug 28, 2018
1fa1be3
Change logic in dc_model, correct pvwatts keyword
cwhanse Aug 28, 2018
121a8b1
More adjustment to pvwatts_dc vs pvwatts keyword
cwhanse Aug 28, 2018
10a5f7d
Add handling of invalid model name string
cwhanse Aug 29, 2018
c0f58c3
Add test for invalid dc model parameters
cwhanse Aug 30, 2018
ae3d572
Fix typo in cec_dc_adr_ac_system
cwhanse Aug 30, 2018
e11d1c5
Add to whatsnew.rst
cwhanse Aug 30, 2018
743c621
Cleanup, remove copy from test
cwhanse Aug 30, 2018
669d4bd
Delete comment from infer_dc_model
cwhanse Aug 30, 2018
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
46 changes: 43 additions & 3 deletions pvlib/modelchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,16 +365,56 @@ def dc_model(self, model):
elif isinstance(model, str):
model = model.lower()
if model == 'sapm':
self._dc_model = self.sapm
# validate module parameters
missing_params = self._sapm_param_set() - \
set(self.system.module.keys())
Copy link
Member

Choose a reason for hiding this comment

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

self.system.module_parameters.keys()

if missing_params: # some parameters are not in module.keys()
raise ValueError('SAPM selected for the DC model but ' + \
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 you can get by without the + \. Strings are automatically concatenated within () that span multiple lines.

'the module is missing one or more ' + \
'required parameters: ' + \
str(missing_params))
else:
self._dc_model = self.sapm
elif model == 'singlediode':
self._dc_model = self.singlediode
# validate module parameters
missing_params = self._singlediode_param_set() - \
set(self.system.module.keys())
if missing_params: # some parameters are not in module.keys()
raise ValueError('singlediode selected for the DC ' + \
'model but the module is missing one ' + \
'or more required parameters: ' + \
str(missing_params))
else:
self._dc_model = self.singlediode
elif model == 'pvwatts':
self._dc_model = self.pvwatts_dc
# validate module parameters
missing_params = self._pvwatts_dc_param_set() - \
set(self.system.module.keys())
if missing_params: # some parameters are not in module.keys()
raise ValueError('pvwatts_dc selected for the DC ' + \
'model but the module is missing one ' + \
'or more required parameters: ' + \
str(missing_params))
else:
self._dc_model = self.pvwatts_dc
else:
raise ValueError(model + ' is not a valid DC power model')
else:
self._dc_model = partial(model, self)

def _sapm_param_set():
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with using a function for each of these but it seems a bit overkill and harder to extend in the future.

One alternative would be defining a class or module level dictionary like this:

dc_model_parameters = {'sapm': set([....]), 'singlediode': set([...]), 'pvwatts': set([...])}

Copy link
Member

Choose a reason for hiding this comment

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

One could argue that this parameter dictionary should live in pvsystem.py and imported into modelchain.py

Copy link
Member Author

Choose a reason for hiding this comment

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

At what point in modelchain.py should this function be imported?

Copy link
Member

Choose a reason for hiding this comment

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

Probably at the top with the rest of the imports.

return set(['A0', 'A1', 'A2', 'A3', 'A4', 'B0', 'B1', 'B2', 'B3', 'B4',
'B5', 'C0', 'C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'Isco',
'Impo', 'Aisc', 'Aimp', 'Bvoco', 'Mbvoc', 'Bvmpo', 'Mbvmp',
'N', 'Cells_in_Series', 'IX0', 'IXX0', 'FD'])
Copy link
Member

Choose a reason for hiding this comment

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

do you want to use this whole set in the ModelChain.infer_dc_model below?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes


def _singlediode_param_set():
return set(['alpha_sc', 'a_ref', 'I_L_ref', 'I_o_ref', 'R_sh_ref',
'R_s'])

def _pvwatts_dc_param_set():
return set(['pdc0', 'gamma_pdc'])

def infer_dc_model(self):
params = set(self.system.module_parameters.keys())
if set(['A0', 'A1', 'C7']) <= params:
Expand Down