Repository navigation
bifacial.utils.vf_ground_sky_2d interprets max_rows asymmetrically #1867
Description
Activity
- added a commit that references this issue
on Sep 20, 2023 k goes from
-max_rowstomax_rows+1so the symmetry is for x=0 which is indeed problematic if you intergrate betwen 0 and 1 for a smallmax_rows. However asmax_rowsis set to be high enough to neglect the horizon VF, not sure it will change a lot the results.
Reacted by Kevin Anderson, Jules Chéron and Adam R. JensenHi @kandersolar, is this issue still open? I would love to contribute to this issue!
@kandersolar I want to contribute to this issue if anyone had not taken that .
@kandersolar I want to contribute to this issue if anyone had not taken that .
Hi 👋 @Chirag3841 Thanks for your interest in contributing to pvlib. Please note that making meaningful code contributions to pvlib requires some familiarity with photovoltaics or solar irradiance. If you have this background, we would be happy to help you along.
Reacted by Kevin Anderson, Will Holmgren, Echedey Luis and Anton Driessereally what I want is effectively a max_wedges parameter instead of max_rows?
This hints at the real problem, which is that there are several repeating units in this model, not all of which are aligned:
- PV rows
- Ground segments of length
pitch - Air gaps between adjacent PV rows
Unfortunately, the model was defined such that ground segments are not centered on PV rows, i.e. ground segments go from
0topitchrelative to the point underneath a PV row. This problem could have been avoided by defining the ground surface to go from-pitch/2topitch/2instead (although that approach may have its own problems).Consider the following diagram:
The parameter description for
max_rowssays:pvlib-python/pvlib/bifacial/utils.py
Lines 119 to 121 in 603ede0
For this
x=0.8, the "current row" is Row 0. Therefore, with the current implementation, formax_rows=1, we must consider only rows -1 to 1, which means the appropriate VF is the sum of wedges -1 and 0.I think it would be nicer if
max_rowswas defined here relative to the "current ground segment" (instead of "current row"), which would have the following behavior:max_rows=1: consider only Rows 0 and 1, so VF is given by wedge 0.max_rows=2: consider Rows -1 to 2, so VF is given by the sum of wedges -1, 0, and 1.
Any thoughts?
relative to the "current ground segment" (instead of "current row")
+1 to this. But I would exclude the rows that define the current ground segment, so that
max_rows=1is the sum of wedges -1, 0, 1.- added a commit that references this issue
on Jun 1, 2026 - added a commit that references this issue
on Aug 5, 2026 Closed by #2740
Describe the bug
vf_ground_sky_2dconsiders one fewer sky wedge on the right side of the central wedge than it does on the left side.To Reproduce
I added a line
print(next_edge)after here:pvlib-python/pvlib/bifacial/utils.py
Line 170 in 7608eda
Then I ran
_ = vf_ground_sky_2d(surface_tilt=0, gcr=0.5, x=0.5, pitch=1, height=1, max_rows=3)and got the printed text:[[[0.01598559 0.04556841 0.18086881 0.48507125 0.18086881 0.04556841]]].Each value in that output describes the VF of a visible sky wedge. The inputs describe a symmetrical system, so the VFs should be symmetrical too, and they are, except one of them is missing... 0.485 for the wedge directly overhead, with three wedges to the left but only two to the right.
Expected behavior
I want the number of sky wedges considered in each direction to be equal.
Maybe this is a fencepost problem, and really what I want is effectively a
max_wedgesparameter instead ofmax_rows?