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 all commits
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
56 changes: 49 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 @@
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,29 @@
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 : dict (array input) or DataFrame (Series input)
Keys/columns are:
* sky_diffuse (the sum of the components below)
* isotropic
* circumsolar
* horizon
* poa_sky_diffuse (the sum of the components below)
* poa_isotropic
* poa_circumsolar
* poa_horizon
The first four elements will be deprecated in v0.14.0 and are kept
to avoit breaking changes.

References
----------
Expand All @@ -630,9 +649,31 @@
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 = dict()

# original formatting (to be deprecated in v0.14.0)
diffuse_components['sky_diffuse'] = poa_sky_diffuse # total

Check failure on line 658 in pvlib/irradiance.py

View workflow job for this annotation

GitHub Actions / flake8-linter 

E261 at least two spaces before inline comment
diffuse_components['isotropic'] = poa_sky_diffuse
diffuse_components['circumsolar'] = 0
diffuse_components['horizon'] = 0

# new formatting
diffuse_components['poa_sky_diffuse'] = poa_sky_diffuse
diffuse_components['poa_isotropic'] = poa_sky_diffuse
diffuse_components['poa_circumsolar'] = 0
diffuse_components['poa_horizon'] = 0

if isinstance(poa_sky_diffuse, pd.Series):
# follows `perez` worfklow
# shouldn't it include an argument `index=dhi.index`?
diffuse_components = pd.DataFrame(diffuse_components)
Comment on lines +669 to +672
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any comments about my question here? This code is used in perez.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, should preserve the index from dhi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would adding this index bit to the diffuse-component-including perez and haydavies functions be considered a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's a breaking change. pvlib strives that a function returns the same data type as the inputs. That's one reason we test functions with different input types. If those functions are returning e.g. Array for Series input, I'd regard that as a bug that needs fixing.

In perez the index wasn't explicitly set because the upstream calculations resulted in sky_diffuse being a Series. I think it's better to be explicit, just in case the calculation uses some numpy function that casts Series to Array.


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 +823,9 @@
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