Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add diffuse self-shading functions #1017

Merged
merged 13 commits into from
Aug 20, 2020

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Aug 4, 2020

  • Closes #xxxx
  • 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.

This implements a modified version of the diffuse self-shading model in Passias and Kallbach (1984) "SHADING EFFECTS IN ROWS OF SOLAR CELL PANELS", which I believe has an error in the equation for the average masking angle (Eq 9). Here is a gist comparing the paper's equation with the one I derived, along with a numerical integration for comparison: https://gist.github.com/kanderso-nrel/2c6c3a1853338cdef5b4bbc67092ccc8. Note that I implemented two versions of the paper's equation to account for an ambiguous minus sign.

If anyone has a Mathematica license and can produce a cleaner version of that equation, I'd be happy to replace it -- that quick and dirty Maxima script in the docstring is about as much as I can do with symbolic solvers.

@kandersolar
Copy link
Member Author

Test failures appear to be unrelated, a change in pandas: #1018

pvlib/shading.py Outdated Show resolved Hide resolved
pvlib/shading.py Outdated
return np.degrees(psi_avg)


def passias_sky_diffuse(masking_angle):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this signature creates space for other approaches to calculate self-shading. What if the arguments are tilt and 'gcr' and passias_masking_angle was private and called within this function? I doubt that the average "masking angle" is general among other models, I could be wrong.

A thought: how is this visible sky dome calculation done in bifacial_vf? Maybe there's some commonality that could inform the interface here, although we wouldn't re-use code from bifacial_vf. If I recall right, that calculation is done with view factors over small increments of angle.

I have a mild preference for sky_diffuse_passias as the function name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I separated the two functions in anticipation of people wanting to use the "worst-case" angle like SAM does instead of Passias et al.'s average angle. That said, a function that just does 1 - cos(angle/2)^2 does seem strange.

Good idea about bifacial_vf; I haven't had an excuse to get familiar with it before now but am happy to have one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the SAM documentation. I think that the low level functions that calculates the average masking_angle is useful, as would be a function that returns masking_angle at a point along the row's surface. Maybe someone would want to use the masking_angle in combination with other sky diffuse models.

Still chewing on the name passias_sky_diffuse for the effects function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a masking_angle function that calculates the angle for a given point and renamed passias_sky_diffuse to sky_diffuse_passias, but am happy to rename if you have a better alternative.

Maybe someone would want to use the masking_angle in combination with other sky diffuse models.

It occurs to me that the masking angle is also useful for comparing with solar position to determine if direct shading is relevant.


Notes
-----
The pvlib-python authors believe that Eqn. 9 in [1]_ is incorrect.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equation below isn't equivalent to the published equation? Or, you couldn't reproduce the published equation [9]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter. The published equation [9] also gives different results from evaluating the integral numerically: https://gist.github.com/kanderso-nrel/2c6c3a1853338cdef5b4bbc67092ccc8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm persuaded. Your solution looks correct to me. I computed a few points on the k=1 and k=2 curves to check.

pvlib/shading.py Outdated Show resolved Hide resolved
Panel tilt from horizontal [degrees].

gcr : float
The ground coverage ratio of the array [unitless].
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a notion that gcr is sometimes defined as gcr := width * cos(tilt) / pitch for fixed-tilt arrays. Is that a common usage? If so, this parameter description and the one in masking_angle_passias should clarify that width / pitch is meant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience the usual definition of ground coverage ratio is the ratio of module area to ground area, where the "ground area" is left somewhat ambiguous. That's the definition used in Pvsyst, Helioscope and pvlib.tracking.SingleAxisTracker. I think we should stick with gcr = width / pitch. I'd be interested to see where the ratio of projected horizontal width to pitch is used.

pvlib/shading.py Outdated Show resolved Hide resolved
pvlib/shading.py Outdated Show resolved Hide resolved
pvlib/shading.py Outdated
Comment on lines 161 to 166
the top of the shading row. SAM assumes the "worst-case" loss where the
masking angle is calculated for the bottom of the array [1]_. In [2]_
the masking angle is calculated as the average across the module height.

This function, as in [2]_, makes the assumption that sky diffuse
irradiance is isotropic.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entirely editorial - I'd swap the order of the last and next-to-last sentences, so that the focus stays on the Passias approach.

@cwhanse
Copy link
Member

cwhanse commented Aug 20, 2020

@wholmgren @mikofski any objections to merge?

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks @kanderso-nrel

related issues include #399 and #61

@mikofski
Copy link
Member

LGTM too. I made one comment on a trig identity that might be useful, but probably not essential. This is a great addition. Thanks Kevin!

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider substituting the trig identity, but not a blocker.

@mikofski mikofski merged commit 27872b8 into pvlib:master Aug 20, 2020
@kandersolar kandersolar deleted the diffuse_self_shading branch August 20, 2020 17:15
@mikofski mikofski mentioned this pull request Aug 21, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants