Fix silently ignoring file path in pvsystem.retrieve_sam when `name…

…` 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

docs/sphinx/source/whatsnew/v0.10.5.rst

@@ -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
@@ -36,3 +39,5 @@ Requirements
 Contributors
 ~~~~~~~~~~~~
 * Cliff Hansen (:ghuser:`cwhanse`)
+* :ghuser:`apct69`
+* Mark Mikofski (:ghuser:`mikofski`)

pvlib/pvsystem.py

@@ -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
@@ -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:
 
@@ -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
@@ -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
     -------
@@ -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
     -----
@@ -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):

pvlib/tests/test_pvsystem.py

@@ -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):