Skip to content

Commit

Permalink
Fix silently ignoring file path in pvsystem.retrieve_sam when `name…
Browse files Browse the repository at this point in the history 
…` is provided (#2020)

* My approach to the issue

* Deprecate previous parameters

* No reason to over-engineer, right?

* Update v0.10.5.rst

* Update pvsystem.py

* Improve error handling

* Add ppl involved

* kevin's suggestions
  • Loading branch information
@echedey-ls
echedey-ls committed May 1, 2024
1 parent 8668a61 commit ad2e6ac
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 111 deletions.
5 changes: 5 additions & 0 deletions docs/sphinx/source/whatsnew/v0.10.5.rst
View file
Expand Up @@ -15,6 +15,9 @@ Enhancements

Bug fixes
~~~~~~~~~
* Fixed :py:func:`pvlib.pvsystem.retrieve_sam` silently ignoring the `path` parameter
when `name` was provided. Now an exception is raised requesting to only provide one
of the two parameters. (:issue:`2018`, :pull:`2020`)


Testing
Expand All @@ -36,3 +39,5 @@ Requirements
Contributors
~~~~~~~~~~~~
* Cliff Hansen (:ghuser:`cwhanse`)
* :ghuser:`apct69`
* Mark Mikofski (:ghuser:`mikofski`)
84 changes: 46 additions & 38 deletions pvlib/pvsystem.py
View file
Expand Up @@ -7,7 +7,7 @@
import functools
import io
import itertools
import os
from pathlib import Path
import inspect
from urllib.request import urlopen
import numpy as np
Expand Down Expand Up @@ -1958,9 +1958,9 @@ def calcparams_pvsyst(effective_irradiance, temp_cell,


def retrieve_sam(name=None, path=None):
'''
Retrieve latest module and inverter info from a local file or the
SAM website.
"""
Retrieve latest module and inverter info from a file bundled with pvlib,
a path or an URL (like SAM's website).
This function will retrieve either:
Expand All @@ -1971,10 +1971,14 @@ def retrieve_sam(name=None, path=None):
and return it as a pandas DataFrame.
.. note::
Only provide one of ``name`` or ``path``.
Parameters
----------
name : string, optional
Name can be one of:
Use one of the following strings to retrieve a database bundled with
pvlib:
* 'CECMod' - returns the CEC module database
* 'CECInverter' - returns the CEC Inverter database
Expand All @@ -1985,7 +1989,7 @@ def retrieve_sam(name=None, path=None):
* 'ADRInverter' - returns the ADR Inverter database
path : string, optional
Path to the SAM file. May also be a URL.
Path to a CSV file or a URL.
Returns
-------
Expand All @@ -1997,7 +2001,11 @@ def retrieve_sam(name=None, path=None):
Raises
------
ValueError
If no name or path is provided.
If no ``name`` or ``path`` is provided.
ValueError
If both ``name`` and ``path`` are provided.
KeyError
If the provided ``name`` is not a valid database name.
Notes
-----
Expand Down Expand Up @@ -2030,38 +2038,38 @@ def retrieve_sam(name=None, path=None):
CEC_Date NaN
CEC_Type Utility Interactive
Name: AE_Solar_Energy__AE6_0__277V_, dtype: object
'''

if name is not None:
name = name.lower()
data_path = os.path.join(
os.path.dirname(os.path.abspath(__file__)), 'data')
if name == 'cecmod':
csvdata = os.path.join(
data_path, 'sam-library-cec-modules-2019-03-05.csv')
elif name == 'sandiamod':
csvdata = os.path.join(
data_path, 'sam-library-sandia-modules-2015-6-30.csv')
elif name == 'adrinverter':
csvdata = os.path.join(
data_path, 'adr-library-cec-inverters-2019-03-05.csv')
elif name in ['cecinverter', 'sandiainverter']:
# Allowing either, to provide for old code,
# while aligning with current expectations
csvdata = os.path.join(
data_path, 'sam-library-cec-inverters-2019-03-05.csv')
else:
raise ValueError(f'invalid name {name}')
elif path is not None:
if path.startswith('http'):
response = urlopen(path)
csvdata = io.StringIO(response.read().decode(errors='ignore'))
else:
csvdata = path
"""
# error: path was previously silently ignored if name was given GH#2018
if name is not None and path is not None:
raise ValueError("Please provide either 'name' or 'path', not both.")
elif name is None and path is None:
raise ValueError("A name or path must be provided!")

return _parse_raw_sam_df(csvdata)
raise ValueError("Please provide either 'name' or 'path'.")
elif name is not None:
internal_dbs = {
"cecmod": "sam-library-cec-modules-2019-03-05.csv",
"sandiamod": "sam-library-sandia-modules-2015-6-30.csv",
"adrinverter": "adr-library-cec-inverters-2019-03-05.csv",
# Both 'cecinverter' and 'sandiainverter', point to same database
# to provide for old code, while aligning with current expectations
"cecinverter": "sam-library-cec-inverters-2019-03-05.csv",
"sandiainverter": "sam-library-cec-inverters-2019-03-05.csv",
}
try:
csvdata_path = Path(__file__).parent.joinpath(
"data", internal_dbs[name.lower()]
)
except KeyError:
raise KeyError(
f"Invalid name {name}. "
+ f"Provide one of {list(internal_dbs.keys())}."
) from None
else: # path is not None
if path.lower().startswith("http"): # URL check is not case-sensitive
response = urlopen(path) # URL is case-sensitive
csvdata_path = io.StringIO(response.read().decode(errors="ignore"))
else:
csvdata_path = path
return _parse_raw_sam_df(csvdata_path)


def _normalize_sam_product_names(names):
Expand Down
121 changes: 48 additions & 73 deletions pvlib/tests/test_pvsystem.py
View file
Expand Up @@ -103,82 +103,57 @@ def test_PVSystem_get_iam_invalid(sapm_module_params, mocker):
system.get_iam(45, iam_model='not_a_model')


def test_retrieve_sam_raise_no_parameters():
def test_retrieve_sam_raises_exceptions():
"""
Raise an exception if no parameters are provided to `retrieve_sam()`.
Raise an exception if an invalid parameter is provided to `retrieve_sam()`.
"""
with pytest.raises(ValueError) as error:
with pytest.raises(ValueError, match="Please provide either"):
pvsystem.retrieve_sam()
assert 'A name or path must be provided!' == str(error.value)


def test_retrieve_sam_cecmod():
"""
Test the expected data is retrieved from the CEC module database. In
particular, check for a known module in the database and check for the
expected keys for that module.
"""
data = pvsystem.retrieve_sam('cecmod')
keys = [
'BIPV',
'Date',
'T_NOCT',
'A_c',
'N_s',
'I_sc_ref',
'V_oc_ref',
'I_mp_ref',
'V_mp_ref',
'alpha_sc',
'beta_oc',
'a_ref',
'I_L_ref',
'I_o_ref',
'R_s',
'R_sh_ref',
'Adjust',
'gamma_r',
'Version',
'STC',
'PTC',
'Technology',
'Bifacial',
'Length',
'Width',
]
module = 'Itek_Energy_LLC_iT_300_HE'
assert module in data
assert set(data[module].keys()) == set(keys)


def test_retrieve_sam_cecinverter():
"""
Test the expected data is retrieved from the CEC inverter database. In
particular, check for a known inverter in the database and check for the
expected keys for that inverter.
"""
data = pvsystem.retrieve_sam('cecinverter')
keys = [
'Vac',
'Paco',
'Pdco',
'Vdco',
'Pso',
'C0',
'C1',
'C2',
'C3',
'Pnt',
'Vdcmax',
'Idcmax',
'Mppt_low',
'Mppt_high',
'CEC_Date',
'CEC_Type',
]
inverter = 'Yaskawa_Solectria_Solar__PVI_5300_208__208V_'
assert inverter in data
assert set(data[inverter].keys()) == set(keys)
with pytest.raises(ValueError, match="Please provide either.*, not both."):
pvsystem.retrieve_sam(name="this_surely_wont_work", path="wont_work")
with pytest.raises(KeyError, match="Invalid name"):
pvsystem.retrieve_sam(name="this_surely_wont_work")
with pytest.raises(FileNotFoundError):
pvsystem.retrieve_sam(path="this_surely_wont_work.csv")


def test_retrieve_sam_databases():
"""Test the expected keys are retrieved from each database."""
keys_per_database = {
"cecmod": {'Technology', 'Bifacial', 'STC', 'PTC', 'A_c', 'Length',
'Width', 'N_s', 'I_sc_ref', 'V_oc_ref', 'I_mp_ref',
'V_mp_ref', 'alpha_sc', 'beta_oc', 'T_NOCT', 'a_ref',
'I_L_ref', 'I_o_ref', 'R_s', 'R_sh_ref', 'Adjust',
'gamma_r', 'BIPV', 'Version', 'Date'},
"sandiamod": {'Vintage', 'Area', 'Material', 'Cells_in_Series',
'Parallel_Strings', 'Isco', 'Voco', 'Impo', 'Vmpo',
'Aisc', 'Aimp', 'C0', 'C1', 'Bvoco', 'Mbvoc', 'Bvmpo',
'Mbvmp', 'N', 'C2', 'C3', 'A0', 'A1', 'A2', 'A3', 'A4',
'B0', 'B1', 'B2', 'B3', 'B4', 'B5', 'DTC', 'FD', 'A',
'B', 'C4', 'C5', 'IXO', 'IXXO', 'C6', 'C7', 'Notes'},
"adrinverter": {'Manufacturer', 'Model', 'Source', 'Vac', 'Vintage',
'Pacmax', 'Pnom', 'Vnom', 'Vmin', 'Vmax',
'ADRCoefficients', 'Pnt', 'Vdcmax', 'Idcmax',
'MPPTLow', 'MPPTHi', 'TambLow', 'TambHi', 'Weight',
'PacFitErrMax', 'YearOfData'},
"cecinverter": {'Vac', 'Paco', 'Pdco', 'Vdco', 'Pso', 'C0', 'C1', 'C2',
'C3', 'Pnt', 'Vdcmax', 'Idcmax', 'Mppt_low',
'Mppt_high', 'CEC_Date', 'CEC_Type'}
} # fmt: skip
item_per_database = {
"cecmod": "Itek_Energy_LLC_iT_300_HE",
"sandiamod": "Canadian_Solar_CS6X_300M__2013_",
"adrinverter": "Sainty_Solar__SSI_4K4U_240V__CEC_2011_",
"cecinverter": "ABB__PVI_3_0_OUTD_S_US__208V_",
}
# duplicate the cecinverter items for sandiainverter, for backwards compat
keys_per_database["sandiainverter"] = keys_per_database["cecinverter"]
item_per_database["sandiainverter"] = item_per_database["cecinverter"]

for database in keys_per_database.keys():
data = pvsystem.retrieve_sam(database)
assert set(data.index) == keys_per_database[database]
assert item_per_database[database] in data.columns


def test_sapm(sapm_module_params):
Expand Down

0 comments on commit ad2e6ac

Please sign in to comment.