Continuous version of the Perez transposition model implementation #1876

adriesse · 2023-09-26T18:06:02Z

Closes Continuous version of the Perez transposition model #1841
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.

Create a new version of the Perez model having smooth transitions, but without changing the overall performance w.r.t. the original. The solution uses quadratic splines as described in recent presentations and posters, and documented in a paper just accepted for publication in the Solar Energy Journal. (Link will be provided later; the pre-print is available to PR reviewers on request.)

adriesse · 2023-09-27T13:02:37Z

Ok, you can run it now if you want!

I have made the airmass argument optional and could do the same in the original perez. Opinions?

We have recommended an upper limit of 0.9 on F1 in our paper. I think I should put it in the original perez too. Opinions?

I will probably just copy the whole suite of tests for the original perez. Opinions?

cwhanse · 2023-09-27T14:11:55Z

We have recommended an upper limit of 0.9 on F1 in our paper. I think I should put it in the original perez too. Opinions?

Would F1 <= 0.9 be correcting an error or omission in the original paper, or is this a model improvement that has emerged from the recent work?

adriesse · 2023-09-27T14:24:09Z

We have recommended an upper limit of 0.9 on F1 in our paper. I think I should put it in the original perez too. Opinions?

Would F1 <= 0.9 be correcting an error or omission in the original paper, or is this a model improvement that has emerged from the recent work?

Quoting from the paper:

While preparing Figure 2 we noticed that the value of F1
could exceed 1.0 for zenith angles less than 30°, which would
be physically incoherent and produce a negative irradiance for
the isotropic sky component. However, in our validation data
sets, we observed only isolated cases with F1 over 0.7, and for
one equatorial location (the BSRN station DWN) the natural
limit was approximately 0.9. Consequently, there appears to
be a low likelihood of reaching 1.0. Nevertheless, to guarantee
physically coherent transposition results, we recommend that a
limit of 0.9 be imposed on F1 in implementations of both the
original and the continuous models

cwhanse · 2023-09-27T16:19:44Z

Let's modify the perez function in its own PR. It would be nice to have (in GH comments, if not more formally) a comparison of model output before and after the change, to answer "how much difference does this make"

adriesse · 2023-09-28T11:31:46Z

Ok, the main code is a ready. I will not touch the existing perez function.

Comments on whether to duplicate the whole test suite are still welcome.

cwhanse · 2023-09-28T15:03:45Z

Share my thoughts: the implementation changes relatively few lines from irradiance.perez. My reaction was "can this be a kwarg-activated variant in irradiance.perez?" However, I view the Perez-Driesse calculation as distinct from the original Perez model, rather than a variation on the Perez model. The two share a conceptual structure, but differ (importantly) in their parameterization. For this reason, I think it is better to have both irradiance.perez and irradiance.perez_driesse in the API.

adriesse · 2023-09-29T09:44:21Z

The duplicated code in the two function bothers me a bit as well. I would say a future move of the common code to a helper function could be considered, which would not affect the API.

adriesse · 2023-09-29T12:01:28Z

Ok, I've copied all tests but one from perez and added some specific ones for perez_driesse.

Ready for complete review!

cwhanse

I suggest that we add a See Also block to the perez function.

pvlib/irradiance.py

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

adriesse · 2023-10-01T09:06:16Z

I suggest that we add a See Also block to the perez function.

There are several little things that could be mirrored from this function to the old one, I think. A new issue and PR might be best for this.

kandersolar

One comment about the zeta calculation below but otherwise LGTM! There's now also a merge conflict.

pvlib/irradiance.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>

adriesse · 2023-10-10T18:57:53Z

I'm happy to entertain more comments and reviews, but if there are none in the pipeline I would suggest merging so I can continue with things that depend on this.

adriesse · 2023-10-10T19:01:11Z

@kandersolar @cwhanse Regarding the calculations in _calc_zeta, do you think I should add these equations in the journal paper (if still possible).

kandersolar · 2023-10-10T19:33:00Z

Regarding the calculations in _calc_zeta, do you think I should add these equations in the journal paper (if still possible).

I like the paper the way it is now. I think the code comments are sufficient to link these calculations to the paper.

cwhanse · 2023-10-10T19:33:46Z

@kandersolar @cwhanse Regarding the calculations in _calc_zeta, do you think I should add these equations in the journal paper (if still possible).

Yes that would be of value.

adriesse · 2023-10-10T20:03:05Z

I get a merge conflict on those two opinions. :) I'll see if it can be done when @AdamRJensen gets back from Nepal.

adriesse · 2023-10-16T11:34:51Z

Are any further changes needed before merging?

kandersolar · 2023-10-16T13:03:04Z

Thanks @adriesse! (and sorry for needing a reminder to merge this)

* Remove various repeated words in documentation (#1872) * Remove repeated words * Update pvlib/ivtools/sdm.py Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com> --------- Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com> * fix invalid escape sequence '\c' (#1879) * fix invalid escape sequence '\c' pvlib/iam.py:843: DeprecationWarning: invalid escape sequence '\c' Occurence is actually in line 854: `IAM = 1 - (1 - \cos(aoi))^5` * Add to list of contributors * Replace use of deprecated `pkg_resources` (#1881) (#1882) * Update infinite_sheds.py to add shaded fraction to returned variables in infinite_sheds.get_irradiance and infinite_sheds.get_irradiance_poa (#1871) * Update infinite_sheds.py Added shaded fraction to returned variables. * Update v0.10.3.rst * Update test_infinite_sheds.py added tests for shaded fraction * Update test_infinite_sheds.py Corrected the shaded fraction tests in the haydavies portion. * Update pvlib/bifacial/infinite_sheds.py Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com> * Update infinite_sheds.py * Update infinite_sheds.py * Update infinite_sheds.py fixed indentation issues --------- Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com> * Continuous version of the Perez transposition model implementation (#1876) * Definitely not ready for review! * Big step forward. * Add entry in docs. * A working model but just one test sofar. * Add new model as option in get_sky_diffuse. Docstring edits pending. * Completed doc strings. Also a bit of fine-tuning code. * Updated whatsnew. * Bugfix, formatting fix, and add all tests. * Test warning plus some other small changes. * Make flake happy. * Update pvlib/irradiance.py Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * Address comments. * Add contributor code comments. * Update pvlib/irradiance.py Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com> * Adapt to reviewer preferences. * Adapt to flake preferences. * Remove model pseudo-option. * Flake --------- Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com> * Fix spurious test error with pandas 2.1 (#1891) pandas-dev/pandas#55014 * Fix plotting in plot_singlediode.py gallery page (#1895) * Update plot_singlediode.py fixed plot annotations by moving plt.show() further down * Update whatsnew.rst * Update v0.10.3.rst * Update docs/sphinx/source/whatsnew.rst Undoing changes to whatsnew.rst * Address pandas FutureWarnings in test suite (#1900) * Cahnged expected reference in test_detect_clearskY_window to 1 from True to avoid Futurewarning * Change reference to etr in ibird function to avoid FutureWarning * In test_modelchain, update all instances when referring to series by position to using iloc to get rid of FutureWarning * Update to iloc method for referencing by position in test_irradiance to get rid of FutureWarning * In test_singlediode change applymap to map to get rid of FutureWarning * Test_srml update to select using iloc to get rid of FutureWarning * Substitute changing to float64 dtype using map with base functionality that's accessible across Pandas versions * Added username to Contributors * Update line break in test_clearsky to adhere to line length limit * add comparisons to other tools * Apply suggestions from code review Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * revision re: other open-source projects * bibtex tweaks * clarify pvlib matlab comparison --------- Co-authored-by: Miroslav Šedivý <6774676+eumiro@users.noreply.github.com> Co-authored-by: Arjan Keeman <akeeman@users.noreply.github.com> Co-authored-by: Miguel Sánchez de León Peque <peque@neosit.es> Co-authored-by: Will Hobbs <45701090+williamhobbs@users.noreply.github.com> Co-authored-by: Anton Driesse <anton.driesse@pvperformancelabs.com> Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com> Co-authored-by: matsuobasho <rkoulikov@pm.me>

Definitely not ready for review!

d03f97c

adriesse marked this pull request as draft September 26, 2023 18:06

adriesse added 3 commits September 26, 2023 23:52

Big step forward.

4a84218

Add entry in docs.

4d739de

A working model but just one test sofar.

0cbd420

adriesse marked this pull request as ready for review September 27, 2023 12:57

Add new model as option in get_sky_diffuse. Docstring edits pending.

9f39dd5

adriesse added 2 commits September 28, 2023 13:18

Completed doc strings. Also a bit of fine-tuning code.

7848148

Updated whatsnew.

c00029b

cwhanse added the enhancement label Sep 28, 2023

cwhanse added this to the v0.10.3 milestone Sep 28, 2023

Bugfix, formatting fix, and add all tests.

f08bd2f

adriesse added 2 commits September 29, 2023 13:05

Test warning plus some other small changes.

4382f8a

Make flake happy.

5af34e8

cwhanse approved these changes Sep 29, 2023

View reviewed changes

pvlib/irradiance.py Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

adriesse and others added 2 commits September 30, 2023 14:26

Update pvlib/irradiance.py

93a7c99

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

Address comments.

48ec542

Add contributor code comments.

702f088

kandersolar reviewed Oct 4, 2023

View reviewed changes

pvlib/irradiance.py Outdated Show resolved Hide resolved

Merge branch 'main' into transpose

d55049c

AdamRJensen approved these changes Oct 5, 2023

View reviewed changes

pvlib/irradiance.py Outdated Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

pvlib/irradiance.py Outdated Show resolved Hide resolved

adriesse and others added 3 commits October 5, 2023 16:18

Update pvlib/irradiance.py

177e637

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>

Adapt to reviewer preferences.

10e844c

Adapt to flake preferences.

2c702f0

adriesse added 2 commits October 10, 2023 22:21

Remove model pseudo-option.

d004387

Flake

90b84e2

kandersolar merged commit af8dde5 into pvlib:main Oct 16, 2023
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continuous version of the Perez transposition model implementation #1876

Continuous version of the Perez transposition model implementation #1876

adriesse commented Sep 26, 2023 •

edited

Loading

adriesse commented Sep 27, 2023

cwhanse commented Sep 27, 2023

adriesse commented Sep 27, 2023

cwhanse commented Sep 27, 2023

adriesse commented Sep 28, 2023

cwhanse commented Sep 28, 2023

adriesse commented Sep 29, 2023

adriesse commented Sep 29, 2023

cwhanse left a comment

adriesse commented Oct 1, 2023

kandersolar left a comment

adriesse commented Oct 10, 2023

adriesse commented Oct 10, 2023

kandersolar commented Oct 10, 2023

cwhanse commented Oct 10, 2023

adriesse commented Oct 10, 2023

adriesse commented Oct 16, 2023

kandersolar commented Oct 16, 2023

Continuous version of the Perez transposition model implementation #1876

Continuous version of the Perez transposition model implementation #1876

Conversation

adriesse commented Sep 26, 2023 • edited Loading

adriesse commented Sep 27, 2023

cwhanse commented Sep 27, 2023

adriesse commented Sep 27, 2023

cwhanse commented Sep 27, 2023

adriesse commented Sep 28, 2023

cwhanse commented Sep 28, 2023

adriesse commented Sep 29, 2023

adriesse commented Sep 29, 2023

cwhanse left a comment

Choose a reason for hiding this comment

adriesse commented Oct 1, 2023

kandersolar left a comment

Choose a reason for hiding this comment

adriesse commented Oct 10, 2023

adriesse commented Oct 10, 2023

kandersolar commented Oct 10, 2023

cwhanse commented Oct 10, 2023

adriesse commented Oct 10, 2023

adriesse commented Oct 16, 2023

kandersolar commented Oct 16, 2023

adriesse commented Sep 26, 2023 •

edited

Loading