Skip to content

Adapts irradiance.isotropic to return_components framework. #2527

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adapts irradiance.isotropic to return_components framework. #2527

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07ae57e
Adapts irradiance.isotropic to return_components framework.
ramaroesilva Aug 3, 2025
a2916b3
Implements new strategy for output formatting.
ramaroesilva Aug 7, 2025
c633188
Adjusts flake8 formatting.
ramaroesilva Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions pvlib/irradiance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ def get_ground_diffuse(surface_tilt, ghi, albedo=.25, surface_type=None):
return diffuse_irrad


def isotropic(surface_tilt, dhi):
def isotropic(surface_tilt, dhi, return_components=False):
r'''
Determine diffuse irradiance from the sky on a tilted surface using
the isotropic sky model.
Expand All @@ -613,10 +613,23 @@ def isotropic(surface_tilt, dhi):
dhi : numeric
Diffuse horizontal irradiance. [Wm⁻²] DHI must be >=0.

return_components : bool, default False
Flag used to decide whether to return the calculated diffuse components
or not. If `False`, ``sky_diffuse`` is returned. If `True`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
or not. If `False`, ``sky_diffuse`` is returned. If `True`,
or not. If `False`, ``poa_sky_diffuse`` is returned. If `True`,

Assuming this is in reference to the names in the Returns section

``diffuse_components`` is returned.

Returns
-------
diffuse : numeric
The sky diffuse component of the solar radiation.
poa_sky_diffuse : numeric
The sky diffuse component of the solar radiation on a tilted
surface.

diffuse_components : OrderedDict (array input) or DataFrame (Series input)
Keys/columns are:
* sky_diffuse: Total sky diffuse
* isotropic
* circumsolar
* horizon

Copy link
Member

Choose a reason for hiding this comment

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

Update with poa_ prefix

References
----------
Expand All @@ -630,9 +643,20 @@ def isotropic(surface_tilt, dhi):
Energy vol. 201. pp. 8-12
:doi:`10.1016/j.solener.2020.02.067`
'''
sky_diffuse = dhi * (1 + tools.cosd(surface_tilt)) * 0.5
poa_sky_diffuse = dhi * (1 + tools.cosd(surface_tilt)) * 0.5

return sky_diffuse
if return_components:
diffuse_components = OrderedDict()
Copy link
Member

Choose a reason for hiding this comment

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

Minor note: no need to use OrderedDict in new code (#1684)

diffuse_components['poa_sky_diffuse'] = poa_sky_diffuse

# Calculate the different components
diffuse_components['poa_isotropic'] = poa_sky_diffuse
diffuse_components['poa_circumsolar'] = 0
diffuse_components['poa_horizon'] = 0

return diffuse_components
else:
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to return both poa_sky_diffuse and diffuse_components, if return_componets is True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. This "one or the other" is what is implemented in perez at the moment.

For clarity, do you mean returning two dictionaries, which makes the number of outputs depend on return_components, or having still one dictionary buth with an additional sky_diffuse key?

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 thinking a tuple with two dicts, when return_components is True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwhanse I just noticed that in my latest commit I included in the diffuse_components dict the sky_diffuse as one of the keys. Would this - having a single-dict with components and their sum - make sense?

If instead we have two dictionaries as originally proposed, the one having the sky_diffuse will have a single key... How would we call the variable and the key without being repetitive? total_diffuse["sky"]?

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I misspoke. Return a tuple (sky_diffuse, components) where sky_diffuse is the current numeric (Series, array, float) and components is a dict.

Copy link
Member

@cwhanse cwhanse Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Edited: returning a tuple requires the user to change from

sky_diffuse = isotropic(...)

to

sky_diffuse, _ = isotropic(...)

I was (wrongly) thinking that only the first element of the tuple would be assigned to sky_diffuse. Not thinking clearly.

So now I'm not so sure that returning a tuple is a good idea. Still, it seems better than requiring the user to change to

out = isotropic(...)
sky_diffuse = out['sky_diffuse']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying and thinking things through.

Going back to the start:

I would prefer to return both poa_sky_diffuse and diffuse_components, if return_componets is True.

I just remembered that in my latest commit the sky_diffuse is included within diffuse_components, following the workflow used in perez and haydavies. So what we are discussing here is in fact if we should change the current modus operandi of transposition models with return_components.

While what you propose is v0.13.1-friendly for the models not yet returning return_components, it would break previous code for those that already do. Having this said, I would suggest moving this specific topic to a separate issue and raise some discussion aiming for v0.14.

What do you think? If agreed, can you can raise a new issue like @kandersolar did in #2529?

Copy link
Member

Choose a reason for hiding this comment

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

I just remembered that in my latest commit the sky_diffuse is included within diffuse_components, following the workflow used in perez and haydavies. So what we are discussing here is in fact if we should change the current modus operandi of transposition models with return_components.

All the irradiance component sets I looked at include both components and sums of components. I think the cleanest way would be (would have been) to include only the lowest level and leave the summation up to the user.

return poa_sky_diffuse


def klucher(surface_tilt, surface_azimuth, dhi, ghi, solar_zenith,
Expand Down Expand Up @@ -782,8 +806,9 @@ def haydavies(surface_tilt, surface_azimuth, dhi, dni, dni_extra,
or supply ``projection_ratio``.

return_components : bool, default `False`
If `False`, ``sky_diffuse`` is returned.
If `True`, ``diffuse_components`` is returned.
Flag used to decide whether to return the calculated diffuse components
or not. If `False`, ``sky_diffuse`` is returned. If `True`,
``diffuse_components`` is returned.

Returns
--------
Expand Down
Loading