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

reepoi · 2023-03-19T18:49:44Z

Partially addresses Behavior of calcparams and singlediode function could be improved #1626 (first two checkboxes)
I am familiar with the contributing guidelines
Tests added
~~[ ] Updates entries in docs/sphinx/source/reference for API changes.~~
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

This PR addresses the observations about pvsystems.calcparams_* in #1626. These are that pd.Series return values can inherit the name of the pd.Series passed to the functions, and that the numeric return values (either scalar, np.ndarray, pd.Series) are not always the same Python type.

A helper function is added to pvlib/tools.py to get the first pandas index from a list of variables.

cwhanse

I think we need to check the all-float case.

How does test_PVSystem_calcparams_desoto pass? My guess is that calcparams_desoto returning all arrays as expected, and numpy's assert_allclose is doing some work for us, e.g.,

assert_allclose(Rs, 0.1, atol=0.1)

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

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

wholmgren

This PR has changed a lot! I like the simplicity of the latest approach.

wholmgren · 2023-06-07T19:41:18Z

pvlib/pvsystem.py

+    numeric_args = (effective_irradiance, temp_cell)
+    out = (IL, I0, Rs, Rsh, nNsVth)
+
+    if all(map(np.isscalar, numeric_args)):


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.

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)].

wholmgren · 2023-06-07T19:41:21Z

pvlib/pvsystem.py

+    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)


two alternatives here.

get_pandas_index raises exception (TypeError?) instead of returning None, this code uses try/except/else

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.

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.

wholmgren · 2023-06-07T19:44:09Z

pvlib/tests/test_tools.py

+def test_get_pandas_index(args, item_idx):
+    pd.testing.assert_index_equal(
+        args[item_idx].index, tools.get_pandas_index(*args)
+    )


should be tested with all scalars and/or arrays too

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

wholmgren · 2023-06-07T19:52:09Z

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

+  :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`)


"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.

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.

…ed Python type

…eturn-values

pvlib/tests/test_pvsystem.py

…calars

kandersolar · 2023-06-23T22:10:52Z

Thanks @reepoi!

reepoi added 15 commits March 18, 2023 09:44

two decorators for renaming pd.Series in a function

f36d832

calcparams_* Rs return value to match temp_cell type

8d184fe

adding tests for pd.Series naming and resistance_series type conversion

2842a77

methods for converting between numeric-type values

95553fb

adding more tests and clarifying documentation

c188743

rename pdSeries clear name decorator and move it to pvlib.tools

993243e

adding support for scalar to np.ndarray of any shape

44a2a6a

updating match_shape docstring

fabb4eb

fixing syntax error

3cb6845

fix stickler issues

96b558b

Merge branch 'main' of github.com:pvlib/pvlib-python into return-values

aeaf065

different approach

0547ed7

remove unnecessary asserts

dd2f64b

Merge branch 'main' of github.com:pvlib/pvlib-python into return-values

ac91f3b

remove file

8612500

reepoi changed the title ~~Helper functions for consistent Python types of numeric return values.~~ Consistent return types for pvsystem.calcparams_* May 24, 2023

reepoi changed the title ~~Consistent return types for pvsystem.calcparams_*~~ Matching Python types for return values of pvsystem.calcparams_* May 24, 2023

update whatsnew

43945f3

reepoi marked this pull request as ready for review May 24, 2023 17:36

cwhanse added the enhancement label May 31, 2023

cwhanse added this to the 0.10.0 milestone May 31, 2023

cwhanse reviewed May 31, 2023

View reviewed changes

docs/sphinx/source/whatsnew/v0.10.0.rst Outdated Show resolved Hide resolved

reepoi and others added 2 commits June 7, 2023 09:05

add tests for all scalar inputs, and pass np.arrays to assert_allclose

d86bc10

Update docs/sphinx/source/whatsnew/v0.10.0.rst

5d4f6d4

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

wholmgren reviewed Jun 7, 2023

View reviewed changes

reepoi added 3 commits June 8, 2023 11:03

if condition swap, tests for tools.get_pandas_index, tests for return…

05a7de6

…ed Python type

Merge branch 'return-values' of github.com:reepoi/pvlib-python into r…

6bfc361

…eturn-values

fix typo in docstring

5e4c16c

kandersolar mentioned this pull request Jun 12, 2023

v0.10.0 release plan #1735

Closed

kandersolar reviewed Jun 12, 2023

View reviewed changes

pvlib/tests/test_pvsystem.py Outdated Show resolved Hide resolved

pvlib/tests/test_pvsystem.py Show resolved Hide resolved

reepoi added 2 commits June 13, 2023 09:08

correct calcparams_cec test, and add test for calcparams_pvsyst all s…

0ff0f45

…calars

Merge branch 'main' of github.com:pvlib/pvlib-python into return-values

370ae3c

cwhanse approved these changes Jun 21, 2023

View reviewed changes

kandersolar approved these changes Jun 23, 2023

View reviewed changes

kandersolar merged commit 0bc5a53 into pvlib:main Jun 23, 2023
28 of 29 checks passed

kandersolar mentioned this pull request Jun 23, 2023

Behavior of calcparams and singlediode function could be improved #1626

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

reepoi commented Mar 19, 2023 •

edited by cwhanse

cwhanse left a comment •

edited

wholmgren left a comment

wholmgren Jun 7, 2023

reepoi Jun 8, 2023

wholmgren Jun 7, 2023

reepoi Jun 8, 2023

wholmgren Jun 7, 2023

reepoi Jun 8, 2023

wholmgren Jun 7, 2023

reepoi Jun 8, 2023 •

edited

kandersolar commented Jun 23, 2023

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

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

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

Conversation

reepoi commented Mar 19, 2023 • edited by cwhanse

cwhanse left a comment • edited

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

wholmgren Jun 7, 2023

Choose a reason for hiding this comment

reepoi Jun 8, 2023

Choose a reason for hiding this comment

wholmgren Jun 7, 2023

Choose a reason for hiding this comment

reepoi Jun 8, 2023

Choose a reason for hiding this comment

wholmgren Jun 7, 2023

Choose a reason for hiding this comment

reepoi Jun 8, 2023

Choose a reason for hiding this comment

wholmgren Jun 7, 2023

Choose a reason for hiding this comment

reepoi Jun 8, 2023 • edited

Choose a reason for hiding this comment

kandersolar commented Jun 23, 2023

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

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

reepoi commented Mar 19, 2023 •

edited by cwhanse

cwhanse left a comment •

edited

reepoi Jun 8, 2023 •

edited