Implement ANTS-2D bifacial irradiance model#2740
Conversation
echedey-ls
left a comment
There was a problem hiding this comment.
Some first impressions; also, you may want to link to it in other bifacial "See also" sections.
| def vf_ground_sky_2d_integ(surface_tilt, gcr, height, pitch, max_rows=10, | ||
| npoints=100, vectorize=False): | ||
| @renamed_kwarg_warning("0.15.2", "surface_tilt", "tracker_rotation") | ||
| def vf_ground_sky_2d_integ(tracker_rotation, gcr, height, pitch, g0=0, g1=1, |
There was a problem hiding this comment.
| def vf_ground_sky_2d_integ(tracker_rotation, gcr, height, pitch, g0=0, g1=1, | |
| def vf_ground_sky_2d_integ(tracker_rotation, gcr, height, pitch, *, g0=0, g1=1, |
The rationale is that previous versions assume this call signature
vf_ground_sky_2d_integ(0, 0.5, 2, 4, 20, 200), where max_rows=20 and npoints=200
This wouldn't fail in the new version [1], where values g0 and g1 would take 20 and 200 respectively.
[1] At this branch
>>> import pvlib
>>> pvlib.bifacial.utils.vf_ground_sky_2d_integ(0, 0.5, 2, 4, 20, 200)
array(1.14662412e-05)
[2] On main
array([0.49984351])
There was a problem hiding this comment.
Making these keyword-only arguments would be breaking, which makes me reluctant to take it on here. @echedey-ls how about the alternative of moving g0 and g1 to the end of the signature so that existing usage is not affected?
There was a problem hiding this comment.
That should work, yes, assuming nobody suppresses the warnings by setting those old params to None. Cause whenever they get deleted, then that function call would fail. I can only think of an approach that allows code to never fail, but that requires a deprecation period for positional params max_rows and npoints prior to merging this PR. [1, 2]
Your suggestion seems to be the most agile, the use-case I propose to be an absolute edge-case, and I prefer to merge this PR in next minor release rather than to make it wait for two minor versions.
[1] Decorator for deprecating positional-allowed params https://github.com/pyvista/pyvista/blob/main/pyvista/_deprecate_positional_args.py
[2] Example usage of [1] https://github.com/pyvista/pyvista/blob/21dd07fb3444356c61aacaf83510ba71cebd9780/pyvista/plotting/helpers.py#L65
| def vf_row_ground_2d_integ(surface_tilt, gcr, height=None, pitch=None, | ||
| x0=0, x1=1, g0=0, g1=1, max_rows=20): |
There was a problem hiding this comment.
I'd say this one is also subject to the problem described in previous comment
cwhanse
left a comment
There was a problem hiding this comment.
I'm OK reviewing the whole PR, if we can do it in a sequence of smaller review bites.
| # TODO seems like this should be np.arange(-max_rows, max_rows+1)? | ||
| # see GH #1867 | ||
| k = np.arange(-max_rows, max_rows)[:, np.newaxis, np.newaxis] |
There was a problem hiding this comment.
I would do (-max_rows, max_rows + 1) or is this a breaking change needing deprecation?
Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>
| surface_tilt : numeric | ||
| Surface tilt angle in degrees from horizontal, e.g., surface facing up | ||
| = 0, surface facing horizon = 90. [degree] | ||
| tracker_rotation : numeric |
There was a problem hiding this comment.
I'm not persuaded that tracker_rotation and surface_tilt are interchangeable. When axis_tilt is not 0 the rotation angle is not the same as the angle from horizontal. In this case, the 2D geometry being considered is in the plane perpendicular to the tracker rotation axis, not perpendicular to the ground. Does that matter?
Also, with tracker_rotation as the input, this public function relies on context outside the scope of the function: axis_tilt and axis_aximuth are needed to know what tracker_rotation is. If we need the input to be tracker_rotation then maybe consider making this function private.
There was a problem hiding this comment.
I'm not persuaded that
tracker_rotationandsurface_tiltare interchangeable. Whenaxis_tiltis not 0 the rotation angle is not the same as the angle from horizontal. In this case, the 2D geometry being considered is in the plane perpendicular to the tracker rotation axis, not perpendicular to the ground. Does that matter?
I think tracker_rotation is correct here:
- Adding
g0andg1(rather than implicitly hardcoding them as 0/1) means that we can't assume symmetry around the ground midpoint anymore. That means that we need to distinguish east- and west-facing rotations of the same tilt, so a signed quantity liketracker_rotationis needed. - The VF must be calculated from the ground's point of view. When
axis_tiltis nonzero, that means the ground surface is tilted too (the model assumes that the tracking axis is parallel to the ground surface). In the reference frame aligned with the ground,tracker_rotationis the requiredsurface_tiltanalog.
Also, with
tracker_rotationas the input, this public function relies on context outside the scope of the function:axis_tiltandaxis_azimuthare needed to know whattracker_rotationis. If we need the input to betracker_rotationthen maybe consider making this function private.
Yeah, I guess it is a little awkward. Making the function private seems a shame, since it's useful in other contexts. Would noting the geometric link between tracker_rotation, g0, and g1 be a sufficient alternative? g0 and g1 are documented using left and right, so maybe something like:
tracker_rotation : numeric
Tracker rotation angle. Positive rotations indicate raising the
row's right edge. [degree]
By the way, we are talking about vf_ground_sky_2d_integ, but the existing vf_ground_sky_2d signature has the same issue (tracker rotation input, but no axis tilt/azimuth): https://pvlib-python.readthedocs.io/en/stable/reference/generated/pvlib.bifacial.utils.vf_ground_sky_2d.html
| ------- | ||
| fgnd_sky : numeric | ||
| Integration of view factor over the length between adjacent, interior | ||
| rows. Shape matches that of ``surface_tilt``. [unitless] |
There was a problem hiding this comment.
Replace surface_tilt or not, pending outcome of comment about tracker_rotation
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
|
Is there a reason that |
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
The only reason is that I did not think to include it in the reference paper, so including it here would be going beyond the reference. If no reviewers object to that in this case, I'd be fine adding it in (and maybe documenting it as an extension). |
That sounds great to me! |
[ ] Closes #xxxxdocs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.@AdamRJensen, @cwhanse, and I have a paper describing a new bifacial irradiance model called ANTS-2D. It is similar to pvlib's
infinite_shedsmodel, but extended to allow:Details available open-access here: https://doi.org/10.1109/JPHOTOV.2026.3677506
This PR is rather large. To summarize:
g0andg1parameters to the view factor functions inpvlib.bifacial.utils. These are analogous tox0andx1invf_row_sky_2d_integand extend the functions to subset the ground surface.vf_ground_sky_2d_integto use Hottel's crossed-string rule instead of burdensome numerical integration. This makes thenpointsandvectorizeparameters unnecessary.pvlib.bifacial.utilsto be cleaner with the new calculations.pvlib.bifacial.infinite_shedsto accommodate theutilschangespvlib.bifacial.ant2d, which houses the model itself and uses the newutilsfunctionality.Let me know if it would help reviewers to split it up and review separate PRs, starting with
utils.