Update infinite_sheds.py to add shaded fraction to returned variables in infinite_sheds.get_irradiance and infinite_sheds.get_irradiance_poa #1871

williamhobbs · 2023-09-21T18:20:31Z

Added shaded fraction to returned variables in infinite_sheds.get_irradiance and infinite_sheds.get_irradiance_poa.

Related discussion is here #1869.

Closes Return shaded fraction values from infinite_sheds.get_irradiance and get_irradiance_poa #1870
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.

I have relatively little experience with PRs and the checklist above, so I'll likely need some guidance.

Added shaded fraction to returned variables.

kandersolar · 2023-09-22T12:37:21Z

Thanks @williamhobbs, these code changes seem right to me. This PR will need some testing in pvlib/tests/bifacial/test_infinite_sheds.py to verify that the new columns exist and have the expected values, and a note in the v0.10.3 what's new file (you may have to update your branch with changes from "upstream" for this file to exist).

A side note about procedure: you opened this PR based on changes to your main branch, which isn't necessarily a problem here, but comes with the downside of not being able to open more than one PR (a single branch can only have one set of changes), and adding commits to your main means it has a different history from pvlib's main, which might be a headache for you in the future. In general it is better to start a new branch for each PR. Since this one is already opened, it makes sense to just continue on with this one. But after this PR is merged, you might want to re-fork pvlib and start fresh to get a clean main branch again.

williamhobbs · 2023-09-22T13:46:56Z

@kandersolar, thanks for the tip on branches. I was actually wondering about that when I created a branch for this PR...

I've updated the what's new file.

A question on testing: Does this mean that in this part of test_get_irradiance_poa,

https://github.com/williamhobbs/pvlib-python/blob/192a556bade8d18c2758946a8c1218bef6722906/pvlib/tests/bifacial/test_infinite_sheds.py#L96-L105

I need to add something like,

expected_shaded_fraction = np.array([0.2]) # 0.2 is a placeholder, need to add the right value
assert np.isclose(res['shaded_fraction'], expected_shaded_fraction)

and then do something similar further down for test_get_irradiance?

kandersolar · 2023-09-22T13:52:20Z

Yep, that's the idea. Might as well add similar checks in the array/Series sections too.

added tests for shaded fraction

williamhobbs · 2023-09-22T14:49:56Z

Ok, I added tests to pvlib/tests/bifacial/test_infinite_sheds.py.

For all of the combinations of solar position and array geometry that were included in the testing, shaded fraction is always zero. Is that ok? I could change, e.g., solar_zenith and surface_tilt to get a non-zero shaded fraction, but that would also change expected values for most/all other outputs. I was also thinking I could add a new test function like test_shaded_fraction_in_get_irradiance that only checks shaded_fraction in a case with shade, but I'm not sure if that's the right way to setup tests...

Corrected the shaded fraction tests in the haydavies portion.

kandersolar · 2023-09-22T14:55:09Z

I was also thinking I could add a new test function like test_shaded_fraction_in_get_irradiance that only checks shaded_fraction in a case with shade, but I'm not sure if that's the right way to setup tests...

That would be fine. In this case I think it's okay to not add that test since the functionality of the shaded fraction calculation is already tested elsewhere (e.g. test__shaded_fraction_array). Tests for this PR are more to ensure that the calculation results are being returned as expected, less about the actual values themselves.

williamhobbs · 2023-09-22T15:50:50Z

After fixing a copy-paste mistake, it looks like I got the testing working. Can I check the boxes above for updating entries for API changes (which I don't think is relevant here?), documentation, and ready for detailed review?

kandersolar · 2023-09-22T15:56:47Z

Yep! You can also ~~strikethrough~~ items that don't apply by wrapping in ~ like this: ~Updates entries...~.

kandersolar

LGTM!

My only comment is that someone not familiar with the model might be surprised that shaded_fraction_back is zero most of the time. Perhaps the docs should specify that this column represents specifically row-to-row shade, and not "sun is behind the POA" shade? Fine with me to leave as is though.

williamhobbs · 2023-09-28T17:29:08Z

My only comment is that someone not familiar with the model might be surprised that shaded_fraction_back is zero most of the time. Perhaps the docs should specify that this column represents specifically row-to-row shade, and not "sun is behind the POA" shade? Fine with me to leave as is though.

I'd be fine with that, but I might defer to you, @kandersolar, or @mikofski about how to describe that clearly.

kandersolar

One idea for parameter description wording below.

Also @williamhobbs there's a merge conflict now that pvlib:main has been updated by other PRs in the time since this one was opened. You can resolve the conflict locally using git commands and your text editor or using GitHub's tool at the bottom of this PR page.

pvlib/bifacial/infinite_sheds.py

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

williamhobbs · 2023-10-04T14:05:20Z

Thanks, @kandersolar. I think I've updated/fixed everything.

pvlib/bifacial/infinite_sheds.py

mikofski

Is this @williamhobbs first PR? Coincidence that it’s one of my favorites of all time?! Congrats Will 🎉🥳

fixed indentation issues

williamhobbs · 2023-10-04T15:37:23Z

Flake8 caught my indentation mistakes. I just corrected those... (and yes, @mikofski, this is my first pvlib PR!)

kandersolar · 2023-10-04T16:05:01Z

Thanks @williamhobbs!

* 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>

Update infinite_sheds.py

ffebd7a

Added shaded fraction to returned variables.

kandersolar added the enhancement label Sep 22, 2023

kandersolar added this to the v0.10.3 milestone Sep 22, 2023

williamhobbs added 2 commits September 22, 2023 08:07

Merge branch 'pvlib:main' into main

c9a9403

Update v0.10.3.rst

192a556

Update test_infinite_sheds.py

754b09c

added tests for shaded fraction

Update test_infinite_sheds.py

4237ab6

Corrected the shaded fraction tests in the haydavies portion.

kandersolar approved these changes Sep 22, 2023

View reviewed changes

williamhobbs mentioned this pull request Sep 29, 2023

Implement PV self-shade calculations SolarArbiter/solarforecastarbiter-core#820

Open

kandersolar reviewed Oct 4, 2023

View reviewed changes

pvlib/bifacial/infinite_sheds.py Outdated Show resolved Hide resolved

williamhobbs and others added 3 commits October 4, 2023 08:56

Update pvlib/bifacial/infinite_sheds.py

2f136b6

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

Update infinite_sheds.py

1b6b511

Merge branch 'main' into main

7ae70a5

cwhanse reviewed Oct 4, 2023

View reviewed changes

pvlib/bifacial/infinite_sheds.py Outdated Show resolved Hide resolved

Update infinite_sheds.py

1382187

cwhanse approved these changes Oct 4, 2023

View reviewed changes

mikofski approved these changes Oct 4, 2023

View reviewed changes

Update infinite_sheds.py

f3a67da

fixed indentation issues

kandersolar merged commit 46851d9 into pvlib:main Oct 4, 2023

kandersolar mentioned this pull request Dec 11, 2023

Return shaded fraction values from infinite_sheds.get_irradiance and get_irradiance_poa #1870

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 to add shaded fraction to returned variables in infinite_sheds.get_irradiance and infinite_sheds.get_irradiance_poa #1871

williamhobbs commented Sep 21, 2023 •

edited

Loading

kandersolar commented Sep 22, 2023

williamhobbs commented Sep 22, 2023

kandersolar commented Sep 22, 2023

williamhobbs commented Sep 22, 2023

kandersolar commented Sep 22, 2023

williamhobbs commented Sep 22, 2023

kandersolar commented Sep 22, 2023

kandersolar left a comment

williamhobbs commented Sep 28, 2023

kandersolar left a comment

williamhobbs commented Oct 4, 2023

mikofski left a comment

williamhobbs commented Oct 4, 2023 •

edited

Loading

kandersolar commented Oct 4, 2023

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 to add shaded fraction to returned variables in infinite_sheds.get_irradiance and infinite_sheds.get_irradiance_poa #1871

Conversation

williamhobbs commented Sep 21, 2023 • edited Loading

kandersolar commented Sep 22, 2023

williamhobbs commented Sep 22, 2023

kandersolar commented Sep 22, 2023

williamhobbs commented Sep 22, 2023

kandersolar commented Sep 22, 2023

williamhobbs commented Sep 22, 2023

kandersolar commented Sep 22, 2023

kandersolar left a comment

Choose a reason for hiding this comment

williamhobbs commented Sep 28, 2023

kandersolar left a comment

Choose a reason for hiding this comment

williamhobbs commented Oct 4, 2023

mikofski left a comment

Choose a reason for hiding this comment

williamhobbs commented Oct 4, 2023 • edited Loading

kandersolar commented Oct 4, 2023

williamhobbs commented Sep 21, 2023 •

edited

Loading

williamhobbs commented Oct 4, 2023 •

edited

Loading