Implement ANTS-2D bifacial irradiance model#2740
Implement ANTS-2D bifacial irradiance model#2740kandersolar wants to merge 48 commits intopvlib:mainfrom
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.
| Projected solar zenith angle, defined around the same axis as | ||
| ``tracker_rotation``. [degree]. |
There was a problem hiding this comment.
I don't understand this definition. A vector can be projected onto a plane or another vector. An angle is formed by two vectors. "around" suggests an orientation of a vector.
tan_phi was defined in the docstrings for _solar_project_tangent as
Tangent of the angle between the zenith vector and the sun vector
projected to the plane defined by the zenith vector and surface_azimuth.
So I would expect phi to be the angle of the sun vector projected to a plane formed by....
Since phi isn't clear, tracker_rotation becomes unclear also.
There was a problem hiding this comment.
I edited to Projected solar zenith angle, defined as a right-handed rotation around the same axis as ``tracker_rotation`` , paralleling the description of tracker_rotation. I would not object to taking the approach of the original tan_phi though.
There was a problem hiding this comment.
The angle phi is a rotation? Or is the projection of vectors that form phi somehow a rotation? I'm still not seeing this clearly.
There was a problem hiding this comment.
How about
phi : numeric
Projected solar zenith angle, defined as a right-handed rotation on
the plane perpendicular to the rotation axis, with respect to horizontal. [degree].
?
Both phi and tracker_rotation are currently defined based one on another, which may make it extra difficult to see. I would add rotation axis to the definition of tracker_rotation too. And clarify later (notes section?, the same params definitions?, IDK) to interpret fixed-tilt as an specific case of SAT.
| # 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?
There was a problem hiding this comment.
I went through and fixed all these to what I think makes sense. Which + constant is appropriate depends on the function. I also went ahead and fixed #1867, calling it a bugfix.
Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>
[ ] 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.