From ad2e6ac4ae1be27cea2ab946047dd0f42d445a8f Mon Sep 17 00:00:00 2001 From: Echedey Luis <80125792+echedey-ls@users.noreply.github.com> Date: Wed, 1 May 2024 18:26:51 +0200 Subject: [PATCH] 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 | 5 + pvlib/pvsystem.py | 84 ++++++++-------- pvlib/tests/test_pvsystem.py | 121 ++++++++++-------------- 3 files changed, 99 insertions(+), 111 deletions(-) diff --git a/docs/sphinx/source/whatsnew/v0.10.5.rst b/docs/sphinx/source/whatsnew/v0.10.5.rst index 9bbb8c9745..41c55da8f9 100644 --- a/docs/sphinx/source/whatsnew/v0.10.5.rst +++ b/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`) diff --git a/pvlib/pvsystem.py b/pvlib/pvsystem.py index e0037f30fc..edd5bd3952 100644 --- a/pvlib/pvsystem.py +++ b/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): diff --git a/pvlib/tests/test_pvsystem.py b/pvlib/tests/test_pvsystem.py index 52de0fcc5b..1151432bd7 100644 --- a/pvlib/tests/test_pvsystem.py +++ b/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):