Skip to content
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

Fix silently ignoring file path in pvsystem.retrieve_sam when name is provided #2020

Merged
merged 8 commits into from
May 1, 2024
Merged

Fix silently ignoring file path in pvsystem.retrieve_sam when name is provided #2020

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
16c0ee1
My approach to the issue
echedey-ls Apr 27, 2024
4ea8fb0
Deprecate previous parameters
echedey-ls Apr 27, 2024
a2b446f
No reason to over-engineer, right?
echedey-ls Apr 27, 2024
405de69
Update v0.10.5.rst
echedey-ls Apr 27, 2024
5b4aa4b
Update pvsystem.py
echedey-ls Apr 27, 2024
bce44f4
Improve error handling
echedey-ls Apr 28, 2024
215c038
Add ppl involved
echedey-ls Apr 29, 2024
a8d1d15
kevin's suggestions
echedey-ls Apr 29, 2024
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
3 changes: 3 additions & 0 deletions docs/sphinx/source/whatsnew/v0.10.5.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
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 Down
86 changes: 49 additions & 37 deletions pvlib/pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
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,13 @@ 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.
ValueError
If the provided ``name`` is not valid.
ValueError
If the provided ``path`` does not exist.

Notes
-----
Expand Down Expand Up @@ -2030,38 +2040,40 @@ 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')
"""
# 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("Please provide either 'name' or 'path'.")
elif name is not None:
internal_dbs = {
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but anyone consider using a constant INTERNAL_DBS at the top of the module instead of defining it on every call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like something only used in a function is better placed near it, and I doubt the extra computation time does save significant time when there will be two calls at most in a script.

"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",
}
name_lwr = name.lower()
if name_lwr in internal_dbs.keys(): # input is a database name
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 this extra call is unnecessary, just use a try except or better yet let it raise key error:

try: 
    cvsdata = internal_dbs[name_lwr]
except KeyError as exc:
    raise exc  # or do what you want
else:  # only executes if no exception
    csvdata_path = Path(__file__).parent.joinpath(
                "data", cvsdata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. You've inspired me to avoid checking the existence of the file too.

csvdata_path = Path(__file__).parent.joinpath(
Copy link
Member

Choose a reason for hiding this comment

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

In general beware of placing too many executions in try-except or else isolating exceptions can be more difficult. Best to limit one operation with known exceptions

"data", internal_dbs[name_lwr]
)
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'))
raise ValueError(
f"Invalid name {name}. Provide one of {internal_dbs.keys()}."
echedey-ls marked this conversation as resolved.
Show resolved Hide resolved
)
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"))
elif Path(path).exists():
csvdata_path = path
else:
csvdata = path
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(f"Invalid path {path}: does not exist.")
return _parse_raw_sam_df(csvdata_path)


def _normalize_sam_product_names(names):
Expand Down
114 changes: 44 additions & 70 deletions pvlib/tests/test_pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,82 +103,56 @@ 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)
with pytest.raises(ValueError, match="Please provide either.*, not both."):
pvsystem.retrieve_sam(name="this_surely_wont_work", path="wont_work")
with pytest.raises(ValueError, match="Invalid name"):
pvsystem.retrieve_sam(name="this_surely_wont_work")
with pytest.raises(ValueError, match="Invalid path"):
pvsystem.retrieve_sam(path="this_surely_wont_work.csv")


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)
"""Test the expected keys are retrieved from each database."""
echedey-ls marked this conversation as resolved.
Show resolved Hide resolved
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
keys_per_database["sandiainverter"] = keys_per_database["cecinverter"]
module_per_database = {
echedey-ls marked this conversation as resolved.
Show resolved Hide resolved
"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_",
"sandiainverter": "ABB__PVI_3_0_OUTD_S_US__208V_"
}

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


def test_sapm(sapm_module_params):
Expand Down
Loading