Skip to content

Matching Python types for return values of pvsystem.calcparams_* #1700

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 23 commits into from
Jun 23, 2023
Merged

Matching Python types for return values of pvsystem.calcparams_* #1700

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f36d832
two decorators for renaming pd.Series in a function
reepoi Mar 18, 2023
8d184fe
calcparams_* Rs return value to match temp_cell type
reepoi Mar 18, 2023
2842a77
adding tests for pd.Series naming and resistance_series type conversion
reepoi Mar 18, 2023
95553fb
methods for converting between numeric-type values
reepoi Mar 19, 2023
c188743
adding more tests and clarifying documentation
reepoi Mar 19, 2023
993243e
rename pdSeries clear name decorator and move it to pvlib.tools
reepoi Mar 19, 2023
44a2a6a
adding support for scalar to np.ndarray of any shape
reepoi Mar 19, 2023
fabb4eb
updating match_shape docstring
reepoi Mar 19, 2023
3cb6845
fixing syntax error
reepoi Mar 22, 2023
96b558b
fix stickler issues
reepoi Mar 22, 2023
aeaf065
Merge branch 'main' of github.com:pvlib/pvlib-python into return-values
reepoi May 24, 2023
0547ed7
different approach
reepoi May 24, 2023
dd2f64b
remove unnecessary asserts
reepoi May 24, 2023
ac91f3b
Merge branch 'main' of github.com:pvlib/pvlib-python into return-values
reepoi May 24, 2023
8612500
remove file
reepoi May 24, 2023
43945f3
update whatsnew
reepoi May 24, 2023
d86bc10
add tests for all scalar inputs, and pass np.arrays to assert_allclose
reepoi Jun 7, 2023
5d4f6d4
Update docs/sphinx/source/whatsnew/v0.10.0.rst
reepoi Jun 7, 2023
05a7de6
if condition swap, tests for tools.get_pandas_index, tests for return…
reepoi Jun 8, 2023
6bfc361
Merge branch 'return-values' of github.com:reepoi/pvlib-python into r…
reepoi Jun 8, 2023
5e4c16c
fix typo in docstring
reepoi Jun 8, 2023
0ff0f45
correct calcparams_cec test, and add test for calcparams_pvsyst all s…
reepoi Jun 13, 2023
370ae3c
Merge branch 'main' of github.com:pvlib/pvlib-python into return-values
reepoi Jun 13, 2023
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
4 changes: 4 additions & 0 deletions docs/sphinx/source/whatsnew/v0.10.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ Deprecations

Enhancements
~~~~~~~~~~~~
* The return values of :py:func:`pvlib.pvsystem.calcparams_desoto`,
:py:func:`pvlib.pvsystem.calcparams_cec`, and
:py:func:`pvlib.pvsystem.calcparams_pvsyst` are all numeric types and have
the same Python type. (:issue:`1626`, :pull:`1700`)


Bug fixes
Expand Down
32 changes: 27 additions & 5 deletions pvlib/pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from pvlib import (atmosphere, iam, inverter, irradiance,
singlediode as _singlediode, temperature)
from pvlib.tools import _build_kwargs, _build_args
import pvlib.tools as tools


# a dict of required parameter names for each DC power model
Expand Down Expand Up @@ -1913,7 +1914,7 @@ def calcparams_desoto(effective_irradiance, temp_cell,
saturation_current : numeric
Diode saturation curent in amperes

resistance_series : float
resistance_series : numeric
Series resistance in ohms

resistance_shunt : numeric
Expand Down Expand Up @@ -2036,9 +2037,20 @@ def calcparams_desoto(effective_irradiance, temp_cell,
# use errstate to silence divide by warning
with np.errstate(divide='ignore'):
Rsh = R_sh_ref * (irrad_ref / effective_irradiance)

Rs = R_s

return IL, I0, Rs, Rsh, nNsVth
numeric_args = (effective_irradiance, temp_cell)
out = (IL, I0, Rs, Rsh, nNsVth)

if all(map(np.isscalar, numeric_args)):
Copy link
Member

@wholmgren wholmgren Jun 7, 2023

Choose a reason for hiding this comment

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

np.isscalar docs suggest that we should instead test if np.ndim(x) == 0. The most likely point of failure is example table row 2: np.isscalar(np.array(3.1)). numpy scalar arrays seem to pop up all over the place.

Copy link
Contributor Author

@reepoi reepoi Jun 8, 2023

Choose a reason for hiding this comment

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

With the goal of calcparams_* methods' return values having the same Python type, I think we want the behavior of np.isscalar instead of np.ndim(x) == 0. If calcparams_* is given a mix of floats and 0d-arrays (e.g. effective_irradiance is a Python float and temp_cell is a 0d-array), using np.ndim(x) == 0 will cause the calcparams_* methods' return values to be a mix of floats and 0d-arrays. If we use np.isscalar, all return values will become 0d-arrays. This is because np.broadcast_arrays(1.0, np.array(2.0)) equals [np.array(1.0), np.array(2.0)].

return out

index = tools.get_pandas_index(*numeric_args)
if index is not None:
return tuple(pd.Series(a, index=index).rename(None) for a in out)

return np.broadcast_arrays(*out)
Copy link
Member

@wholmgren wholmgren Jun 7, 2023

Choose a reason for hiding this comment

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

two alternatives here.

  1. get_pandas_index raises exception (TypeError?) instead of returning None, this code uses try/except/else
  2. return None, but swap the clauses, so if index is none: return np.broadcast_arrays; else: return tuple(...)

option 1 feels more pythonic, but not sure that it's actually better. If keeping None, then I think option 2 is marginally cleaner.

Copy link
Contributor Author

@reepoi reepoi Jun 8, 2023

Choose a reason for hiding this comment

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

I went with your second alternative. get_pandas_index can also be used here in pvsystem.singlediode, but using it there would become awkward if get_pandas_index raised an exception. The calcparams_ functions change their behavior depending on whether get_pandas_index returns None so a try/except pattern makes sense. However, in the pvsystem.singlediode use case I linked, I want to use the None value when creating the pd.DataFrame.



def calcparams_cec(effective_irradiance, temp_cell,
Expand Down Expand Up @@ -2117,7 +2129,7 @@ def calcparams_cec(effective_irradiance, temp_cell,
saturation_current : numeric
Diode saturation curent in amperes

resistance_series : float
resistance_series : numeric
Series resistance in ohms

resistance_shunt : numeric
Expand Down Expand Up @@ -2234,7 +2246,7 @@ def calcparams_pvsyst(effective_irradiance, temp_cell,
saturation_current : numeric
Diode saturation current in amperes

resistance_series : float
resistance_series : numeric
Series resistance in ohms

resistance_shunt : numeric
Expand Down Expand Up @@ -2293,7 +2305,17 @@ def calcparams_pvsyst(effective_irradiance, temp_cell,

Rs = R_s

return IL, I0, Rs, Rsh, nNsVth
numeric_args = (effective_irradiance, temp_cell)
out = (IL, I0, Rs, Rsh, nNsVth)

if all(map(np.isscalar, numeric_args)):
return out

index = tools.get_pandas_index(*numeric_args)
if index is not None:
return tuple(pd.Series(a, index=index).rename(None) for a in out)

return np.broadcast_arrays(*out)


def retrieve_sam(name=None, path=None):
Expand Down
75 changes: 43 additions & 32 deletions pvlib/tests/test_pvsystem.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,26 +746,30 @@ def test_Array__infer_cell_type():

def test_calcparams_desoto(cec_module_params):
times = pd.date_range(start='2015-01-01', periods=3, freq='12H')
effective_irradiance = pd.Series([0.0, 800.0, 800.0], index=times)
temp_cell = pd.Series([25, 25, 50], index=times)
df = pd.DataFrame({
'effective_irradiance': [0.0, 800.0, 800.0],
'temp_cell': [25, 25, 50]
}, index=times)

IL, I0, Rs, Rsh, nNsVth = pvsystem.calcparams_desoto(
effective_irradiance,
temp_cell,
alpha_sc=cec_module_params['alpha_sc'],
a_ref=cec_module_params['a_ref'],
I_L_ref=cec_module_params['I_L_ref'],
I_o_ref=cec_module_params['I_o_ref'],
R_sh_ref=cec_module_params['R_sh_ref'],
R_s=cec_module_params['R_s'],
EgRef=1.121,
dEgdT=-0.0002677)
df['effective_irradiance'],
df['temp_cell'],
alpha_sc=cec_module_params['alpha_sc'],
a_ref=cec_module_params['a_ref'],
I_L_ref=cec_module_params['I_L_ref'],
I_o_ref=cec_module_params['I_o_ref'],
R_sh_ref=cec_module_params['R_sh_ref'],
R_s=cec_module_params['R_s'],
EgRef=1.121,
dEgdT=-0.0002677
)

assert_series_equal(IL, pd.Series([0.0, 6.036, 6.096], index=times),
check_less_precise=3)
assert_series_equal(I0, pd.Series([0.0, 1.94e-9, 7.419e-8], index=times),
check_less_precise=3)
assert_allclose(Rs, 0.094)
assert_series_equal(Rs, pd.Series([0.094, 0.094, 0.094], index=times),
check_less_precise=3)
assert_series_equal(Rsh, pd.Series([np.inf, 19.65, 19.65], index=times),
check_less_precise=3)
assert_series_equal(nNsVth, pd.Series([0.473, 0.473, 0.5127], index=times),
Expand All @@ -774,27 +778,31 @@ def test_calcparams_desoto(cec_module_params):

def test_calcparams_cec(cec_module_params):
times = pd.date_range(start='2015-01-01', periods=3, freq='12H')
effective_irradiance = pd.Series([0.0, 800.0, 800.0], index=times)
temp_cell = pd.Series([25, 25, 50], index=times)
df = pd.DataFrame({
'effective_irradiance': [0.0, 800.0, 800.0],
'temp_cell': [25, 25, 50]
}, index=times)

IL, I0, Rs, Rsh, nNsVth = pvsystem.calcparams_cec(
effective_irradiance,
temp_cell,
alpha_sc=cec_module_params['alpha_sc'],
a_ref=cec_module_params['a_ref'],
I_L_ref=cec_module_params['I_L_ref'],
I_o_ref=cec_module_params['I_o_ref'],
R_sh_ref=cec_module_params['R_sh_ref'],
R_s=cec_module_params['R_s'],
Adjust=cec_module_params['Adjust'],
EgRef=1.121,
dEgdT=-0.0002677)
df['effective_irradiance'],
df['temp_cell'],
alpha_sc=cec_module_params['alpha_sc'],
a_ref=cec_module_params['a_ref'],
I_L_ref=cec_module_params['I_L_ref'],
I_o_ref=cec_module_params['I_o_ref'],
R_sh_ref=cec_module_params['R_sh_ref'],
R_s=cec_module_params['R_s'],
Adjust=cec_module_params['Adjust'],
EgRef=1.121,
dEgdT=-0.0002677
)

assert_series_equal(IL, pd.Series([0.0, 6.036, 6.0896], index=times),
check_less_precise=3)
assert_series_equal(I0, pd.Series([0.0, 1.94e-9, 7.419e-8], index=times),
check_less_precise=3)
assert_allclose(Rs, 0.094)
assert_series_equal(Rs, pd.Series([0.094, 0.094, 0.094], index=times),
check_less_precise=3)
assert_series_equal(Rsh, pd.Series([np.inf, 19.65, 19.65], index=times),
check_less_precise=3)
assert_series_equal(nNsVth, pd.Series([0.473, 0.473, 0.5127], index=times),
Expand Down Expand Up @@ -840,12 +848,14 @@ def test_calcparams_cec_extra_params_propagation(cec_module_params, mocker):

def test_calcparams_pvsyst(pvsyst_module_params):
times = pd.date_range(start='2015-01-01', periods=2, freq='12H')
effective_irradiance = pd.Series([0.0, 800.0], index=times)
temp_cell = pd.Series([25, 50], index=times)
df = pd.DataFrame({
'effective_irradiance': [0.0, 800.0],
'temp_cell': [25, 50]
}, index=times)

IL, I0, Rs, Rsh, nNsVth = pvsystem.calcparams_pvsyst(
effective_irradiance,
temp_cell,
df['effective_irradiance'],
df['temp_cell'],
alpha_sc=pvsyst_module_params['alpha_sc'],
gamma_ref=pvsyst_module_params['gamma_ref'],
mu_gamma=pvsyst_module_params['mu_gamma'],
Expand All @@ -861,7 +871,8 @@ def test_calcparams_pvsyst(pvsyst_module_params):
IL.round(decimals=3), pd.Series([0.0, 4.8200], index=times))
assert_series_equal(
I0.round(decimals=3), pd.Series([0.0, 1.47e-7], index=times))
assert_allclose(Rs, 0.500)
assert_series_equal(
Rs.round(decimals=3), pd.Series([0.500, 0.500], index=times))
assert_series_equal(
Rsh.round(decimals=3), pd.Series([1000.0, 305.757], index=times))
assert_series_equal(
Expand Down
14 changes: 14 additions & 0 deletions pvlib/tests/test_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from pvlib import tools
import numpy as np
import pandas as pd


@pytest.mark.parametrize('keys, input_dict, expected', [
Expand Down Expand Up @@ -95,3 +96,16 @@ def test_degrees_to_index_1():
'latitude' or 'longitude' is passed."""
with pytest.raises(IndexError): # invalid value for coordinate argument
tools._degrees_to_index(degrees=22.0, coordinate='width')


@pytest.mark.parametrize('args, item_idx', [
((pd.DataFrame([1], index=[1]),), 0),
((pd.Series([1], index=[1]),), 0),
((1, pd.Series([1], index=[1]),), 1),
((1, pd.Series([1], index=[1]), pd.DataFrame([2], index=[2]),), 1),
((1, pd.DataFrame([1], index=[1]), pd.Series([2], index=[2]),), 1),
])
def test_get_pandas_index(args, item_idx):
pd.testing.assert_index_equal(
args[item_idx].index, tools.get_pandas_index(*args)
)
Copy link
Member

@wholmgren wholmgren Jun 7, 2023

Choose a reason for hiding this comment

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

should be tested with all scalars and/or arrays too

Copy link
Contributor Author

@reepoi reepoi Jun 8, 2023

Choose a reason for hiding this comment

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

I added test cases of all scalars and all arrays where get_pandas_index is expected to return None.

22 changes: 22 additions & 0 deletions pvlib/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,3 +469,25 @@ def _first_order_centered_difference(f, x0, dx=DX, args=()):
# removal in scipy 1.12.0
df = f(x0+dx, *args) - f(x0-dx, *args)
return df / 2 / dx


def get_pandas_index(*args):
"""
Get the index of the first pandas DataFrame or Series in a list of
arguments.

Parameters
----------
args: positional arguments
The numeric values to scan for a pandas index.

Returns
-------
A pandas index or None
None is returned if there are no pandas DataFrames or Series is the
args list.
"""
return next(
(a.index for a in args if isinstance(a, (pd.DataFrame, pd.Series))),
None
)