Added irradiance_loss_pvsyst function to pvsystem #1000

btaute · 2020-07-08T15:48:21Z

Added irradiance_loss_pvsyst function to pvsystem module and method to PVSystem class.

Partially addresses Issue #988.

I still need to incorporate tests and update the "what's new" file. I'm hoping for some feedback on implementation before starting those.

Thank you for your review.

Closes Adding Loss Model Categories #988
I am familiar with the contributing guidelines
Tests added
Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

…o pvsystem class.

cwhanse · 2020-07-09T16:48:24Z

The direction this PR goes is fine. The review will be easier if we can clean up the formatting issues first, LMK if you would like some help with that.

I'm fine with the calculations. I'll probably suggest some renaming of the parameters (drop irradiance_ from each name), and some editing of the docstring.

btaute · 2020-07-09T19:27:38Z

I updated the formatting, but I'm still running into some issues there that I'm not sure how to resolve. Some insight on what's required for passing formatting would be greatly appreciated.

With respect to the "irradiance_" included in many variable names, I agree that's cumbersome and am happy to pull that. The reason I included it was to differentiate from the electrical loss models that can also be associated with shading, soiling, and snow coverage. If you believe that differentiation is obvious given the context of the function, then it's overly specific.

pvlib/pvsystem.py

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

btaute · 2020-07-10T17:06:40Z

I attempted to add tests. I'm a little unfamiliar with this system, but I don't think all of the test failures happening are from my addition. If they are, there is clearly something I'm missing and will require assistance.

I think it's at a place for detailed review when a @pvlib/maintainer has the time.

cwhanse · 2020-07-10T19:41:20Z

The test warnings and failures are unrelated to this PR. I'll review the PR soon.

cwhanse

Thanks @btaute I got caught up in other work. This is looking very good.

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

pvlib/pvsystem.py

pvlib/tests/test_pvsystem.py

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

btaute · 2020-07-22T15:49:04Z

I believe I've incorporated all of your comments @cwhanse. The only one outstanding is the What's New addition. This should be ready for another (maybe final) review.

wholmgren

Thanks @btaute for the contribution. Some questions for you and Cliff below...

Also needs an entry in https://github.com/pvlib/pvlib-python/blob/master/docs/sphinx/source/api.rst

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

pvlib/pvsystem.py

pvlib/tests/test_pvsystem.py

pvlib/pvsystem.py

wholmgren · 2020-07-22T16:17:39Z

pvlib/pvsystem.py

+def irradiance_loss_pvsyst(effective_irradiance, irradiance_shading_loss,
+                           irradiance_snow_loss, irradiance_soiling_loss):


The function would return the same thing if the user permuted the loss arguments. We could consider *losses for a more general and extensible approach.

I like this idea. I believe calling out the losses explicitly helps guide new users, but I think an in-depth tutorial can serve that purpose better while allowing for more flexibility here.

If we make this update, I don't think the function name "irradiance_loss_pvsyst" is apt anymore. The point of the function is that it allows a convenient way to compound any irradiance losses that should be applied to the total POA (not specific to beam, diffuse, or ground POA). Maybe that means it should be called "compound_effective_irradiance_losses?" Maybe "combine_effective_irradiance_losses?"

A future implementation more inline with PVSyst should be a "pvsyst_effective_irradiance" function.

+1 to combine_loss_factors or similar for a general purpose function, since the PVWatts model does the same calculation on a different set of inputs.

I think it's helpful to also have a function named irradiance_loss_pvsyst or similar, with arguments that are easily recognized by someone used to using the PVsyst loss model. If we keep a _pvsyst function, it should return effective_irradiance with losses applied, instead of the loss factor.

effective_irradiance_pvsyst implies that the function also calculates and applies reflection and spectral mismatch modifiers, which is a larger scope that this PR.

pvlib/pvsystem.py

# Conflicts: # pvlib/tests/test_pvsystem.py

btaute · 2020-08-04T19:25:15Z

I'm not sure how to address the Read the Docs build failure. Any advice there would be greatly appreciated.

I changed the direction of the Pull Request based on the conversation. It's now a general function for combining losses, which will end up getting called by almost all of the specific system loss functions (i.e. PVSyst loss functions). The math is simple but it also takes into account resampling, which will be beneficial for extrapolating monthly values to hourly values, etc.

pvlib/pvsystem.py

cwhanse · 2020-08-04T20:13:46Z

@btaute the numerical test failures are unrelated to your PR, @kanderso-nrel reported the cause in #1018

docs/sphinx/source/api.rst

pvlib/pvsystem.py

cwhanse

some suggestions for the docstring

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

wholmgren · 2020-08-28T00:58:34Z

@btaute it looks to me like this could be merged once the conflicts are resolved.

btaute · 2020-09-02T19:15:59Z

@wholmgren I believe those conflicts have been resolved.

wholmgren · 2020-09-02T19:49:28Z

@btaute looks like the merge brought some older unwanted code back into the modules.

btaute · 2020-09-02T20:16:23Z

Sorry, I misunderstood the conflicts. Is this better?

wholmgren · 2020-09-02T21:14:13Z

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

-* Add :py:func:`pvlib.shading.sky_diffuse_passias`,
-  :py:func:`pvlib.shading.masking_angle_passias`, and
-  :py:func:`pvlib.shading.masking_angle` to model diffuse shading loss.
-  (:pull:`1017`)
-* Add :py:func:`pvlib.inverter.fit_sandia` that fits the Sandia inverter model
-  to a set of inverter efficiency curves. (:pull:`1011`)
-* Add :py:func:`pvlib.ivtools.sdm.fit_pvsyst_sandia` and :py:func:`pvlib.ivtools.sdm.fit_desoto_sandia`
-  for fitting the Pvsyst and De Soto models to IV curve data (:issue:`227`)(:pull:`708`)
-* Add factory methods :py:meth:`~pvlib.modelchain.ModelChain.with_pvwatts`
-  :py:meth:`~pvlib.modelchain.ModelChain.with_sapm` to create ``ModelChain``
-  objects configured for the respective modeling paradigms. The
-  configurations are defined in ``modelchain.PVWATTS_CONFIG``, and
-  ``modelchain.SAPM_CONFIG``. (:issue:`1013`, :pull:`1022`)
-* Added *racking_model*, *module_type*, and *temperature_model_parameters* to
-  PVSystem, LocalizedPVSystem, SingleAxisTracker, and
-  LocalizedSingleAxisTracker repr methods. (:issue:`1027`)
-* Added ability for :py:func:`pvlib.soiling.hsu` to accept arbitrary time intervals. (:pull:`980`)


@btaute one more merge issue here and then we'll be done. sorry for the hassle

Okay. I took one more stab at it.

wholmgren · 2020-09-02T22:14:56Z

Thanks @btaute!

Added irradiance_loss_pvsyst function to pvsystem module and method t…

693e60d

…o pvsystem class.

Updated formatting

ad09984

cwhanse reviewed Jul 9, 2020

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

cwhanse reviewed Jul 9, 2020

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

btaute and others added 7 commits July 10, 2020 09:07

fixing loss equation formatting

896e71f

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

Fix math formatting

430ab3d

Added Tests

06efce1

fix formatting in tests

43ac80e

more test formatting fixes

1bd866b

Updated v0.8.0 What's New

7969ce5

Added Issue Number to v0.8.0 What's New

8b617b7

cwhanse reviewed Jul 21, 2020

View reviewed changes

btaute and others added 8 commits July 21, 2020 15:02

Update pvlib/pvsystem.py

2361db2

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

Update pvlib/pvsystem.py

e0e168c

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

Update pvlib/pvsystem.py

b7169bd

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

Update pvlib/tests/test_pvsystem.py

607bdb9

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

Update pvlib/tests/test_pvsystem.py

6c408f5

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

Update pvlib/tests/test_pvsystem.py

4b33177

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

Updated params and output to be percents

29cccdd

fix indents

25fc424

wholmgren reviewed Jul 22, 2020

View reviewed changes

btaute added 4 commits August 4, 2020 14:06

Changed function to generic combine_loss_factors

5d488e7

Changed function to generic combine_loss_factors

6f2f976

Merge branch 'master' of https://github.com/btaute/pvlib-python

3a90af3

# Conflicts: # pvlib/tests/test_pvsystem.py

fixed test typo

6c5e5a9

kandersolar reviewed Aug 4, 2020

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

btaute added 2 commits August 4, 2020 14:41

indentation fix

49a7f04

whitespace fix

50590d3

cwhanse reviewed Aug 4, 2020

View reviewed changes

docs/sphinx/source/api.rst Outdated Show resolved Hide resolved

cwhanse reviewed Aug 4, 2020

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

cwhanse reviewed Aug 4, 2020

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

cwhanse reviewed Aug 4, 2020

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

cwhanse reviewed Aug 4, 2020

View reviewed changes

btaute and others added 5 commits August 4, 2020 16:44

Moved combine_loss_factors to PV System Effects

48fd3b4

Apply suggestions from code review

7107119

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

fill_method docstring update

0b6ae04

Merge branch 'master' of https://github.com/btaute/pvlib-python

ce50282

fill_method docstring update

62e9af3

wholmgren mentioned this pull request Aug 26, 2020

v0.8.0 release planning #955

Closed

wholmgren added this to the 0.8.0 milestone Aug 28, 2020

wholmgren added the enhancement label Aug 28, 2020

Merge branch 'master' into master

0d1e95c

btaute added 3 commits September 2, 2020 15:13

Update v0.8.0.rst

d8abcfe

Update pvsystem.py

4391540

Update test_pvsystem.py

1af4854

wholmgren reviewed Sep 2, 2020

View reviewed changes

Update v0.8.0.rst

70f2fa8

wholmgren merged commit b2a17e4 into pvlib:master Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added irradiance_loss_pvsyst function to pvsystem #1000

Added irradiance_loss_pvsyst function to pvsystem #1000

btaute commented Jul 8, 2020 •

edited by wholmgren

Loading

cwhanse commented Jul 9, 2020

btaute commented Jul 9, 2020

btaute commented Jul 10, 2020

cwhanse commented Jul 10, 2020

cwhanse left a comment

btaute commented Jul 22, 2020

wholmgren left a comment

wholmgren Jul 22, 2020

btaute Jul 22, 2020

cwhanse Jul 24, 2020

btaute commented Aug 4, 2020

cwhanse commented Aug 4, 2020 •

edited

Loading

cwhanse left a comment

wholmgren commented Aug 28, 2020

btaute commented Sep 2, 2020

wholmgren commented Sep 2, 2020

btaute commented Sep 2, 2020

wholmgren Sep 2, 2020

btaute Sep 2, 2020

wholmgren commented Sep 2, 2020

		def irradiance_loss_pvsyst(effective_irradiance, irradiance_shading_loss,
		irradiance_snow_loss, irradiance_soiling_loss):

Added irradiance_loss_pvsyst function to pvsystem #1000

Added irradiance_loss_pvsyst function to pvsystem #1000

Conversation

btaute commented Jul 8, 2020 • edited by wholmgren Loading

cwhanse commented Jul 9, 2020

btaute commented Jul 9, 2020

btaute commented Jul 10, 2020

cwhanse commented Jul 10, 2020

cwhanse left a comment

Choose a reason for hiding this comment

btaute commented Jul 22, 2020

wholmgren left a comment

Choose a reason for hiding this comment

wholmgren Jul 22, 2020

Choose a reason for hiding this comment

btaute Jul 22, 2020

Choose a reason for hiding this comment

cwhanse Jul 24, 2020

Choose a reason for hiding this comment

btaute commented Aug 4, 2020

cwhanse commented Aug 4, 2020 • edited Loading

cwhanse left a comment

Choose a reason for hiding this comment

wholmgren commented Aug 28, 2020

btaute commented Sep 2, 2020

wholmgren commented Sep 2, 2020

btaute commented Sep 2, 2020

wholmgren Sep 2, 2020

Choose a reason for hiding this comment

btaute Sep 2, 2020

Choose a reason for hiding this comment

wholmgren commented Sep 2, 2020

btaute commented Jul 8, 2020 •

edited by wholmgren

Loading

cwhanse commented Aug 4, 2020 •

edited

Loading