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
ramaroesilva wants to merge 6 commits into
base: main
Choose a base branch
from
+38 −7
Open

Adapts irradiance.isotropic to return_components framework. #2527

Changes from all commits
Commits
Show all changes
6 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
796ef8b
Minor change in docstring.
ramaroesilva Dec 21, 2025
b4f779a
Cleans dict create + forces output as dataframe.
ramaroesilva Jan 15, 2026
6350b3c
Updates commits, further restructures isotropic.
ramaroesilva Jan 16, 2026
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
45 changes: 38 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,19 @@ 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 `True`, the resulting dataframe will include three more
columns. If not, only ``poa_sky_diffuse`` column is returned.

Returns
-------
diffuse : numeric
The sky diffuse component of the solar radiation.
poa_sky_diffuse : DataFrame
The sky diffuse component of the solar radiation on a tilted
surface. Columns provided controlled by ``return_components`` argument.
If `False`, ``total`` is returned.
If `True`, ``isotropic``, ``circumsolar`` and ``horizon``.
Index is either inherited from dhi or default.

References
----------
Expand All @@ -630,9 +639,30 @@ 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

return sky_diffuse
poa_isotropic = dhi * (1 + tools.cosd(surface_tilt)) * 0.5

# if pd.DataFrame or Series handles first output as numerical
is_pandas = isinstance(dhi, (pd.DataFrame,pd.Series))
if is_pandas:
poa_isotropic = poa_isotropic.values

poa_sky_diffuse = poa_isotropic

if not return_components:
poa_sky_diffuse = {'total': poa_sky_diffuse}
else:
Copy link
Member

@cwhanse cwhanse Aug 6, 2025

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

@ramaroesilva ramaroesilva Aug 6, 2025

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

@cwhanse cwhanse Aug 6, 2025

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

@ramaroesilva ramaroesilva Aug 6, 2025

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

@cwhanse cwhanse Aug 6, 2025

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

@ramaroesilva ramaroesilva Aug 7, 2025

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

@adriesse adriesse Aug 7, 2025

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.

poa_sky_diffuse = {'isotropic': poa_isotropic,
'circumsolar': 0,
'horizon': 0}

poa_sky_diffuse = pd.DataFrame(poa_sky_diffuse)

# if dhi was pandas, out_df inherits index
if is_pandas:
poa_sky_diffuse.index = dhi.index

return poa_sky_diffuse


def klucher(surface_tilt, surface_azimuth, dhi, ghi, solar_zenith,
Expand Down Expand Up @@ -782,8 +812,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