Extend infinite_sheds to optionally use haydavies transposition

[spaneja]

• 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.

Description
This PR adds functionality for users to use the haydavies sky-diffuse transposition model with the get_irradiance() and get_irradiance_poa() functions in the infinite_sheds engine. The works by calling the haydavies() function inside get_irradiance_poa(), and using its return_components functionality to isolate circumsolar. Once isolated, it is removed from dhi, and incorporated into dni.

I have not completed checkboxes yet, as I believe this PR warrants a larger discussion from the maintainers and community. Mainly - is this method acceptable from a technical perspective, and even if so, is this the right way to implement it (ie - if we think about adding future sky diffuse models, would the implementation be similar and/or would this approach complicate things).

I appreciate feedback and guidence from the community. This is inspired by #1551, and was brought to my attention again with @mikofski's most recent comment from #1553.

[cwhanse]

In my view, shifting circumsolar from DHI to beam is acceptable, but we should make sure to inform users of what is being done for them with the input irradiance, and note this is a departure from the cited reference.

[kandersolar]

Regarding correctness and future expansions, two things come to my mind:

• Adding circumsolar to DNI is a reasonable approximation, but a more conceptually correct treatment would have it come from a disc rather than a point, which would have some small implications for shading and AOI. I'm not sure of the details but I think pvfactors makes some attempt at this.
• Future addition of perez, klucher, or reindl will face additional complication due to infinite_sheds having no concept of a diffuse horizon component.

Neither of those is a blocker here IMHO -- I'm +1 to proceeding with the approach in this PR.

cc @johnMoseleyArray as an interested party

[mikofski]

more conceptually correct treatment would have it come from a disc rather than a point

My understanding from my days in concentrators is that circumsolar is more of an angular distribution, vs a disc of rays parallel to the DNI. I don’t know if that’s what you meant, but the way it’s modeled in SOLTRACE is as rays at a distribution of angles fanning out from like 5mrad to like 20mrad and decaying to zero at some larger angle. This distribution includes DNI within the 5mrad circumsolar disc and is frequently called a sunshape. Maybe that’s what you meant by a ring?

Anyway I have often thought it would be interesting to see “lighter” shadows being cast by circumsolar bending around objects according this distribution, but I think doing this verges on ray tracing which is computationally intense even for simple systems.

[kandersolar]

Sure, I should have said "come from a [nonuniform] disc instead of a point" :) Here's some relevant code in pvfactors: https://github.com/SunPower/pvfactors/blob/master/pvfactors/irradiance/utils.py#L196

[mikofski]

having no concept of a diffuse horizon component.

Does pvfactors have something for this too? @jdnewmil had some ideas about this he shared with me seems right:

horizon diffuse is normally modeled as a ring around the horizon… it is either visible to the surface (at incidence angle equal to tilt) or not (if the tilt is zero).

I wonder if this means that any module with non-zero tilt can always see the horizon (=horizon_irradiance * cos(fixed_tilt)), and that near shade obstructions like adjacent rows don't matter? Also I read this as saying HSAT at solar noon facing skyward have zero horizon component, but have horizon_irradiance * cos(tracker_tilt) all other times.

[cwhanse]

Can we move the discussion of further enhancements (beyond Hay-Davies) to a separate Discussion or Issue? I don't want our contributor to become confused about the scope of this PR :)

[kandersolar]

Thanks @spaneja! It's interesting to use a transposition model this way -- in a way it's more of a decomposition model...

[kandersolar]

I think we should have something like this text somewhere in the docs, but there are a few places it could go (docstring Notes section, docstring model parameter description, user_guide/bifacial.rst). I don't love that there is so much duplication in the get_irradiance and get_irradiance_poa docstrings, but maybe that's just the way it has to be.

If ``model='isotropic'`` (the default), ``dhi`` is assumed to
be isotropically distributed across the sky dome as in [1]_.
This implementation provides an optional extension to [1]_ to
model sky anisotropy: if ``model='haydavies'``, the input ``dhi``
is decomposed into circumsolar and isotropic components using
:py:func:`~pvlib.irradiance.haydavies`, with the circumsolar
component treated as additional ``dni`` for transposition
and shading purposes.  

Besides a bit more docs stuff, I think this PR is looking pretty good.

[cwhanse]

Nicely done @spaneja thank you

[adriesse]

Reading this thread it struck me as rather odd that shifting circumsolar from DHI to beam was discussed as if it were something odd or novel. I thought it was common practice. Maybe I just misread the comments though.

[adriesse]

I think that instead of calling haydavies twice, you should just calculate the anisotropy index here and use the relevant cosines of angles/projection ratios.

[kandersolar]

My 2 cents: maybe it's a bit unthrifty to call haydavies 4x for each get_irradiance call, but being able to do this was part of the motivation for #1553, and I think the simplicity of calling functions in pvlib.irradiance is worth the slight hit to efficiency (which I bet is tiny compared with other parts of infinite_sheds anyway). Also, if we add more options for model in the future, are we going to reimplement all of them here?

[adriesse]

I don't know what the future holds. I do know that in the past and again very recently I have found the functions in pvlib irradiance not suitable for my purposes in a similar way to this example. Each offers a little bundle of calculations, but sometimes I just need them bundled differently.

[jdnewmil]

Do tell. We can learn.

…

On February 23, 2023 11:04:00 AM PST, Anton Driesse ***@***.***> wrote: @adriesse commented on this pull request. > + raise ValueError(f'must supply dni_extra for {model} model') + # Call haydavies first time within the horizontal plane - to subtract + # circumsolar_horizontal from DHI + sky_diffuse_comps_horizontal = haydavies(0, 180, dhi, dni, dni_extra, + solar_zenith, solar_azimuth, + return_components=True) + circumsolar_horizontal = sky_diffuse_comps_horizontal['circumsolar'] + + # Call haydavies a second time where circumsolar_normal is facing + # directly towards sun, and can be added to DNI + sky_diffuse_comps_normal = haydavies(solar_zenith, solar_azimuth, dhi, + dni, dni_extra, solar_zenith, + solar_azimuth, + return_components=True) + circumsolar_normal = sky_diffuse_comps_normal['circumsolar'] + I don't know what the future holds. I do know that in the past and again very recently I have found the functions in pvlib irradiance not suitable for my purposes in a similar way to this example. Each offers a little bundle of calculations, but I need them bundled differently.

-- Sent from my phone. Please excuse my brevity.

[kandersolar]

LGTM. Thanks @spaneja!

[pasquierjb]

Is there another issue about adding Perez transposition model to inifinite_sheds #1668 (comment) ?

[kandersolar]

I don't think there's an issue tracking that idea yet. Feel free to open it :)