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 all 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
5 changes: 5 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 @@ -26,6 +26,11 @@ 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 as the `effective_irradiance` and `temp_cell` parameters. (:issue:`1626`, :pull:`1700`)
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

"the same Python type" - this suggests to me that type(output) == type(input) in every case. This isn't actually tested and I'd be slightly surprised if there are no scalar-ish numpy gotchas.

Copy link
Contributor Author

@reepoi reepoi Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The sentence in the whatsnew is not exactly correct. In this PR, the calcparams_* functions decide the Python type of the return values checking these conditions in order:

  • If arguments passed to calcparams_* are scalars (as determined by np.isscalar), then return values are scalars. Since effective_irradiance and temp_cell are the only numeric-types, and the other arguments are float, this condition only depends on the types of effective_irradiance and temp_cell.
  • If either effective_irradiance or temp_cell is a pd.Series, then return values have type pd.Series.
  • If either effective_irradiance or temp_cell is a nd.ndarray, then return values have type np.ndarray. This includes 0d-arrays.

I added tests to check the return values' Python type here.


* Added `map_variables` parameter to :py:func:`pvlib.iotools.read_srml`
and :py:func:`pvlib.iotools.read_srml_month_from_solardat` (:pull:`1773`)

Expand Down
34 changes: 29 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, spectrum, 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,21 @@ 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

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

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 None:
return np.broadcast_arrays(*out)

return tuple(pd.Series(a, index=index).rename(None) for a in out)


def calcparams_cec(effective_irradiance, temp_cell,
Expand Down Expand Up @@ -2117,7 +2130,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 +2247,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 +2306,18 @@ 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 None:
return np.broadcast_arrays(*out)

return tuple(pd.Series(a, index=index).rename(None) for a in out)


def retrieve_sam(name=None, path=None):
Expand Down
Loading