pvfactors limited implementation for bifacial calculations #635

anomam · 2018-12-14T09:26:29Z

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

Closes bifacial PV system modeling #421
I am familiar with the contributing guidelines.
Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
Implementation of a bifacial model using open-source pvfactors package.

pvlib/irradiance.py

pvlib/test/conftest.py

pvlib/test/test_irradiance.py

anomam · 2018-12-14T09:29:35Z

I just realized that my IDE's autopep8 made a lot of automated changes... 😮 don't hesitate to let me know if that's an issue

cwhanse · 2018-12-14T19:43:52Z

pvlib/irradiance.py

+
+    Parameters
+    ----------
+    solar_azimuth: numpy array of numeric


The rest of pvlib generally uses pandas.Series or pandas.Dataframes. If pvfactors must have np.array as input, can you do some type conversion in this function?

Thanks @cwhanse , I forgot about that convention. So I created a decorating function that does this (with a test for it) in order to separate the modeling logic from the inputs/outputs conversion logic. Let me know if that approach is okay with you

Does pvfactors fail with a Series or DataFrame?

In general, I like the decorator approach if we can make it robust enough to be used throughout pvlib. (Applying the decorator throughout pvlib should be done in a separate PR.)

pvfactors has been tested with numpy arrays so far. But I can try to see if it would work with Series and DataFrames, with some luck duck-typing might work...

Understood; I did implement a test for the decorating function which checks multiple cases. But I can bring the type conversion into the pvfactors function if you prefer. Let me know

pvlib/irradiance.py

cwhanse · 2018-12-14T19:44:57Z

pvlib/irradiance.py

+
+    Returns
+    -------
+    front_poa_irradiance: numpy array of numeric


Output convention could be np.array if inputs are np.array, but pandas.Series if inputs are themselves pandas objects.

Thanks @cwhanse , as mentioned above, the decorating function should handle this request as well. Let me know what you think

pvlib/irradiance.py

cwhanse · 2018-12-14T19:50:52Z

@anomam Thanks for this!

I just realized that my IDE's autopep8 made a lot of automated changes... 😮 don't hesitate to let me know if that's an issue

Yes it is a bit of an issue, although most the changes are improvements. Including them makes it hard to review the scope of the PR when unrelated formatting changes are included. OK to address the formatting in a separate issue/PR with formatting as its scope.

* I used a decorator because I wish to seperate the logic of handling the function inputs and the logic related to using pvfactors

* via function decoration

pvlib/test/conftest.py

anomam · 2018-12-17T07:27:54Z

Hi @cwhanse , thanks again for the initial feedback! I implemented some changes as requested and I think that this PR is getting closer to completion. If possible, I would appreciate some guidance on how to solve the stickler-ci and coverage check failures...

pvlib/test/conftest.py

pvlib/test/test_bifacial.py

setup.py

wholmgren · 2018-12-17T15:51:50Z

pvlib/tools.py

+                new_args.append(new_arg)
+            elif isinstance(arg, pd.DataFrame):
+                is_return_type_pandas = True
+                new_arg = arg.values.ravel()  # make sure that 1D


I think it's better to raise an exception than allow a DataFrame --> 1D array --> Series pattern.

Thanks, I removed the decorator function and implemented checks in the pvfactors function

pvlib/bifacial.py

pvlib/test/conftest.py

* Could not find a version that satisfies the requirement pandas>=0.23.3

* add identical test but using pandas series

pvlib/tools.py

wholmgren

Thanks @anomam for considering our suggestions. This mostly looks good to me. I like the simplicity.

ci/requirements-py27.yml

pvlib/bifacial.py

wholmgren · 2018-12-19T03:14:44Z

pvlib/bifacial.py

+
+    # Select the calculated outputs from the pvrow to observe
+    ipoa_front = df_outputs.loc[:, idx_slice[index_observed_pvrow,
+                                             'front', 'qinc']].values


perhaps only a question because I'm not familiar enough with pvfactors, but why extract .values instead of returning the Series?

I just believed that having a 1D numpy array was simpler for these results, but I can adjust it to pvlib's needs, just let me know!

pvlib/test/test_bifacial.py

cwhanse · 2018-12-19T17:26:42Z

@anomam let me know when you are done with cleanup changes.

One issue is still open in my mind: does bifacial.pvfactors_timeseries return a pandas object (when inputs are pandas) or always return an array (the latter, I think)?

anomam · 2018-12-20T09:37:43Z

Thank you so much for the time you spent on this @cwhanse @wholmgren

@cwhanse, from @wholmgren comments above I got the sense that returning 1D arrays was good, but please don't hesitate to let me know if you prefer pandas series (when the inputs are also pandas series)

I'll take a look again tomorrow (Japan time) to check for any necessary clean up or adjustments

wholmgren · 2018-12-20T16:15:17Z

@cwhanse, from @wholmgren comments above I got the sense that returning 1D arrays was good, but please don't hesitate to let me know if you prefer pandas series (when the inputs are also pandas series)

Sorry, I didn't fully understand the inputs/outputs and discussions (still might not). After looking more carefully, I suggest that the outputs always be pandas Series and DataFrames. My rationale is that

the function is specifically meant for timeseries analysis
the function requires input datetime information (optionally as a DatetimeIndex)
the function always returns a DataFrame for one of its outputs
the returned DataFrame always contains a timeseries column (at least that's my understanding)

Unlike other functions in pvlib that are more flexible, here we require users to handle at least one pandas object (the DataFrame) and we know the timestamps that each row corresponds to. So, I suggest that we always return the two Series with the appropriate DatetimeIndex. My understanding is that the DataFrame would need MultiIndex with (row, timestamp). I think this works: df_registries.set_index(['pvrow', 'timestamp']).

wholmgren · 2018-12-20T16:16:16Z

pvlib/test/test_bifacial.py

+@requires_pvfactors
+def test_pvfactors_timeseries_pandas_inputs():
+    """ Test that pvfactors is functional, using the TLDR section inputs of the
+    package github repo README.md file, but converted to pandas Series"""


link to this would be helpful

wholmgren · 2018-12-20T16:17:08Z

pvlib/test/test_bifacial.py

+        horizon_band_angle=horizon_band_angle,
+        run_parallel_calculations=False, n_workers_for_parallel_calcs=None)
+
+    np.testing.assert_allclose(ipoa_front, expected_ipoa_front,


This should use the pandas test functions if we adopt the always return a Series approach

wholmgren · 2018-12-20T16:19:35Z

pvlib/bifacial.py

+
+    Inputs
+    ------
+    solar_azimuth: numeric (list, numpy array, or pandas Series)


I prefer to only document the input type as numeric (no (list...)). Like it or not, it's consistent with the rest of pvlib.

wholmgren · 2019-01-18T19:56:56Z

I'd like see the PR finished early next week or pushed to 0.6.2. I think we're pretty close, but we need to get 0.6.1 out and this (understandably) has not been touched in awhile.

anomam · 2019-01-19T15:43:47Z

Hi @wholmgren , sorry for the long delay, I'm still on my way back from a lot of travel but I will wrap it up early next week. When is the 0.6.1 release?

anomam · 2019-01-21T10:41:29Z

Hi @wholmgren , I updated the function to return pandas Series and I fixed the docstrings as requested. Let me know if it doesn't look like what you expected. Thanks again for your time

cwhanse · 2019-01-21T16:34:39Z

LGTM. @wholmgren ?

wholmgren

Thanks @anomam. I think we only need to make a minor change to the whatsnew.

wholmgren · 2019-01-21T16:46:06Z

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

@@ -82,6 +84,7 @@ Testing
 ~~~~~~~
 * Add test for :func:`~pvlib.solarposition.hour_angle` (:issue:`597`)
 * Update tests to be compatible with pytest 4.0. (:issue:`623`)
+* Add tests for :py:func:`pvlib.bifacial.pvfactors_timeseries` implementation and :py:func:`pvlib.tools.enforce_numpy_arrays` decorator function (:issue:`421`)


The enforce_numpy_arrays section is outdated.

Thanks, done in 55dd3a6

wholmgren · 2019-01-22T02:49:59Z

Great to see bifacial modeling in pvlib - thanks @anomam!

anomam · 2019-01-22T03:00:23Z

Thank you so much for your help and time spent on this @wholmgren @cwhanse !
I hope to see more bifacial models getting pvlib'ed as well ☀️

kandersolar · 2022-09-29T20:19:08Z

pvlib/bifacial.py

+        rho_front_pvrow=0.03, rho_back_pvrow=0.05,
+        horizon_band_angle=15.,


@anomam do you happen to remember why you chose this particular default value for horizon_band_angle? From what I can tell in the git history, the analogous default on the pvfactors side was 6.5 (both at the time of this PR and today) rather than 15. I know this PR is ancient history at this point so I'm not expecting much, just curious :)

There are now some small differences between this wrapper and pvfactors itself in the rho_* parameters as well, but I think those were only introduced later on after this PR was merged.

hey @kanderso-nrel ! It was a long time ago so I'm not entirely sure anymore, but what I remember is that these values might have needed a bit of tuning to match the measurements depending on the sites etc. I think for the horizon band it might be safe to check what the Perez paper says about what value to use.
For the reflectivity parameter values, it's something I actually never really had time to study in depth. I think the default values for the PV rows were low enough to not have much impact, but that's probably not accurate in all cases...
Sorry if my answers are vague, I think getting better ones would require more work/studies/experience...

anomam added 6 commits December 13, 2018 18:06

Set up import of pvfactors package

80f910f

Add test inputs from github repo README.md TLDR section

8acfd58

Function implementation and testing of outputs

2e982d0

Update docstrings

199ad02

Update pvfactors version to latest v0.1.5

397c850

Merge branch 'master' into pvfactors_implementation

2168e91

stickler-ci bot reviewed Dec 14, 2018

View reviewed changes

cwhanse reviewed Dec 14, 2018

View reviewed changes

pvlib/irradiance.py Outdated Show resolved Hide resolved

cwhanse reviewed Dec 14, 2018

View reviewed changes

pvlib/irradiance.py Outdated Show resolved Hide resolved

anomam added 7 commits December 17, 2018 11:31

Move bifacial calculation and test to new specific modules

1c976e3

Reverting autopep8 changes in irradiance.py and test_irradiance.py

a340c20

Docstrings section name change

fd3adf3

Implement decorator to allow use of pandas objects vs numpy arrays

6435ce3

* I used a decorator because I wish to seperate the logic of handling the function inputs and the logic related to using pvfactors

Update function docstrings since now allowing pandas objects

381000c

* via function decoration

Small docstrings fixes

74b20af

Fix decorator for dataframe case and add test for decorator

89bad6c

stickler-ci bot reviewed Dec 17, 2018

View reviewed changes

pvlib/test/conftest.py Outdated Show resolved Hide resolved

pvlib/test/conftest.py Outdated Show resolved Hide resolved

pvlib/test/conftest.py Outdated Show resolved Hide resolved

Update docs for API changes and whatsnew file

81b8887

stickler-ci bot reviewed Dec 17, 2018

View reviewed changes

pvlib/test/conftest.py Outdated Show resolved Hide resolved

pvlib/test/conftest.py Outdated Show resolved Hide resolved

pvlib/test/conftest.py Outdated Show resolved Hide resolved

Use better naming

418ca72

stickler-ci bot reviewed Dec 17, 2018

View reviewed changes

pvlib/test/conftest.py Outdated Show resolved Hide resolved

pvlib/test/conftest.py Outdated Show resolved Hide resolved

pvlib/test/conftest.py Outdated Show resolved Hide resolved

wholmgren reviewed Dec 17, 2018

View reviewed changes

pvlib/test/test_bifacial.py Outdated Show resolved Hide resolved

wholmgren reviewed Dec 17, 2018

View reviewed changes

setup.py Outdated Show resolved Hide resolved

wholmgren reviewed Dec 17, 2018

View reviewed changes

pvlib/bifacial.py Outdated Show resolved Hide resolved

stickler-ci bot reviewed Dec 18, 2018

View reviewed changes

pvlib/test/conftest.py Outdated Show resolved Hide resolved

anomam added 4 commits December 19, 2018 09:40

Silence stickler and add pvfactors to ci as pip install

7cd7cf9

Fix ci typos + docstrings. And use DatetimeIndex and lists in test

cff21dd

pandas>=0.23.3 not available for py34, so removing pvfactors install

160419a

* Could not find a version that satisfies the requirement pandas>=0.23.3

Remove decorator implementation + check for pd series in function

bd5078b

* add identical test but using pandas series

stickler-ci bot reviewed Dec 19, 2018

View reviewed changes

pvlib/tools.py Outdated Show resolved Hide resolved

Remove unused import

cbc7434

wholmgren reviewed Dec 19, 2018

View reviewed changes

Simplify and clarify pvfactors function

9829b81

wholmgren approved these changes Dec 19, 2018

View reviewed changes

wholmgren reviewed Dec 20, 2018

View reviewed changes

wholmgren mentioned this pull request Jan 19, 2019

scalar inputs yield ndim=0 array outputs #646

Open

anomam added 3 commits January 21, 2019 18:59

Fix docstrings for pvlib consistency + link to pvfactors TLDR

9970179

Merge branch 'master' into pvfactors_implementation

608863f

Always return timestamped series and dataframe and test for that

78756cd

wholmgren reviewed Jan 21, 2019

View reviewed changes

Update whatsnew and remove old part

55dd3a6

wholmgren merged commit 0486c3b into pvlib:master Jan 22, 2019

wholmgren added this to the 0.6.1 milestone Jan 22, 2019

wholmgren added the enhancement label Jan 22, 2019

kandersolar reviewed Sep 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pvfactors limited implementation for bifacial calculations #635

pvfactors limited implementation for bifacial calculations #635

anomam commented Dec 14, 2018 •

edited by wholmgren

Loading

anomam commented Dec 14, 2018

cwhanse Dec 14, 2018

anomam Dec 17, 2018

wholmgren Dec 17, 2018

anomam Dec 18, 2018

cwhanse Dec 14, 2018

anomam Dec 17, 2018

cwhanse commented Dec 14, 2018

anomam commented Dec 17, 2018

wholmgren Dec 17, 2018

anomam Dec 19, 2018

wholmgren left a comment

wholmgren Dec 19, 2018

anomam Dec 19, 2018

cwhanse commented Dec 19, 2018

anomam commented Dec 20, 2018

wholmgren commented Dec 20, 2018

wholmgren Dec 20, 2018

wholmgren Dec 20, 2018

wholmgren Dec 20, 2018

wholmgren commented Jan 18, 2019

anomam commented Jan 19, 2019

anomam commented Jan 21, 2019

cwhanse commented Jan 21, 2019

wholmgren left a comment

wholmgren Jan 21, 2019

anomam Jan 22, 2019

wholmgren commented Jan 22, 2019

anomam commented Jan 22, 2019

kandersolar Sep 29, 2022

anomam Oct 1, 2022

		rho_front_pvrow=0.03, rho_back_pvrow=0.05,
		horizon_band_angle=15.,

pvfactors limited implementation for bifacial calculations #635

pvfactors limited implementation for bifacial calculations #635

Conversation

anomam commented Dec 14, 2018 • edited by wholmgren Loading

pvlib python pull request guidelines

anomam commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Dec 14, 2018

anomam commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Dec 19, 2018

anomam commented Dec 20, 2018

wholmgren commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Jan 18, 2019

anomam commented Jan 19, 2019

anomam commented Jan 21, 2019

cwhanse commented Jan 21, 2019

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Jan 22, 2019

anomam commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anomam commented Dec 14, 2018 •

edited by wholmgren

Loading