DRAFT: Handle dni/dhi=0/0 in irradiance.perez without NaNs by markcampanelli · Pull Request #2808 · pvlib/pvlib-python

markcampanelli · 2026-07-05T18:00:57Z

Inspired by Keep or exclude total diffuse irradiance in the output when return_components=True #2801
I am familiar with the contributing guidelines
I attest that all AI-generated material has been vetted for accuracy and is in compliance with the pvlib license
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.

markcampanelli · 2026-07-05T18:03:17Z

    # eventually be used as indicies for coeffecient look ups
    ebin = np.digitize(eps, (0., 1.065, 1.23, 1.5, 1.95, 2.8, 4.5, 6.2))
    ebin = np.array(ebin)  # GH 642
    ebin[np.isnan(eps)] = 0


@pvlib/maintainers I question if this is wise. Perhaps if a user passes a NaN for DNI or DHI (presumably resulting in a NaN for eps), then the function should fail?

I think nan in -> nan out is the philosophy we try to adhere to. Dunno what makes more sense for this specific case.

K thanks. Sorry I had missed that.

It ain't something to be sorry about, it a legitimate question (the motto, better safe than sorry).

Discussions related to this point:

Consider updating pvlib.tracking.singleaxis() to return zeros (or similar) for nighttime instead of NaNs #2539

Set negative values of AOI loss to 0 rather than np.nan #338

And there was one regarding ModelChain, don't remember exactly where.

I added many more combinations of NaN DNI and DHI to the new tests.

@echedey-ls So in my testing, I realized that if airmass is NaN, then this trumps DNI or DHI being NaN, and the result is zero instead of NaN (including the components, when returned). That sounds like incorrect behavior. Would you agree?

I changed the function's behavior to preserve NaNs inherited from DHI or DNI. New test now passing again.

markcampanelli · 2026-07-05T22:53:25Z

        diffuse_components['poa_horizon'] = dhi * term3

        # Set values of components to 0 when sky_diffuse is 0
        mask = sky_diffuse == 0


@cbcrespo I think this is where one has to be careful about the order of operations when computing the components first and then summing to get the total.

A very edgy case here would be if the components added up to zero, but are themselves not all zero (indeed some components can legitimately be negative in this model). This would likely be exceedingly rare for typical floating point calculations, however.

That said, I think the algorithm semantics would be easier to follow if:

The components were computed first.

The sum of components was computed next.

Finally, the total and components are all set to zero if the sum is (strictly) negative.

Although, if a negative total comes out, should this function raise instead?

(I tend to like things to break obviously rather than fail silently, and a negative total sounds like a model breakage to me, but I could be mis-interpreting the situation.)

On second thought, the "spirit of pvlib-python" might be to set everything to NaN if the total comes out negative, instead of raising.

markcampanelli · 2026-07-05T23:30:06Z

@pvlib/maintainers I have two outstanding issues before moving this out of DRAFT and requesting a full review:

I raise a new FloatingPointError when dividing a non-zero (and non-NaN) DHI by a zero DNI.
I think that if the total sky diffuse POA comes out negative after summing all the components, then we should raise an error OR set this value and all corresponding components to NaN (current behavior sets everything to zero, which I think is significantly misleading).

I think returning NaNs instead of raising is probably closer to what users expect, but I am not sure here. If this is true, then I can change both of the above cases to return NaNs.

cwhanse

@markcampanelli can you populate this table? I'm having trouble following the intent of handling the combinations of 0 and/or NaN

air mass	DHI	DNI
0	0	0
0	0	NaN
0	NaN	0
0	NaN	NaN
NaN	0	0
NaN	0	NaN
NaN	NaN	0
NaN	NaN	NaN

cwhanse · 2026-07-06T14:19:51Z

    surface using the surface tilt angle, surface azimuth angle, diffuse
-    horizontal irradiance, direct normal irradiance, extraterrestrial
-    irradiance, sun zenith angle, sun azimuth angle, and relative (not
-    pressure-corrected) airmass. Optionally a selector may be used to
-    use any of Perez's model coefficient sets.
+    horizontal irradiance (DHI), direct normal irradiance (DNI),
+    extraterrestrial irradiance, sun zenith angle, sun azimuth angle, and
+    relative (not pressure-corrected) airmass. Optionally a selector may be


Let's remove the list of input quantities. That was ported from Matlab before the current docstring format was settled.

markcampanelli · 2026-07-06T18:04:55Z

@markcampanelli can you populate this table? I'm having trouble following the intent of handling the combinations of 0 and/or NaN

case	air mass	DHI	DNI	total	circumsolar	isotropic	horizon
a	42	0	0	0	0	0	0
b	42	0	NaN	NaN	NaN	NaN	NaN
c	42	NaN	0	NaN	NaN	NaN	NaN
d	42	NaN	NaN	NaN	NaN	NaN	NaN
e	NaN	0	0	0	0	0	0
f	NaN	0	NaN	NaN	NaN	NaN	NaN
g	NaN	NaN	0	NaN	NaN	NaN	NaN
h	NaN	NaN	NaN	NaN	NaN	NaN	NaN
i	42	100	0	-22.2 -> NaN	-65.5 -> NaN	37.0 -> NaN	6.34 -> NaN
j	NaN	100	0	NaN or 0?	NaN or 0?	NaN or 0?	NaN or 0?
k	42	0	100	NaN	NaN	NaN	NaN
l	NaN	0	100	NaN or 0?	NaN or 0?	NaN or 0?	NaN or 0?

I added components columns and four rows to the end of your table template.

The first added row shows a negative sky-diffuse total result that is negative, along with examples of components that produce it. The row after this is same input but with NaN airmass.

The last two added rows actually raise a FloatingPointError in my current code, but I think, for consistency, this should change to the NaNs shown (or zeros when airmass is NaN?).

After making this table, it is unclear to me if a NaN airmass should supersede a "nonsensical" DHI+DNI combo, for example, DHI=0 and DNI=100. If "NaN airmass wins", then the output is all zeros, if "nonsensical DHI+DNI" wins, then output is all NaNs. Which to choose?

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

markcampanelli · 2026-07-07T14:59:05Z

@cwhanse If I remove raising the FloatingPointError when, say, dni/dhi = 100/0, then the eps (epsilon) parameter evaluates to inf (following standard IEEE 754), and subsequently the total sky diffuse and all components compute to zero due to how inf is treated in the subsequent lookup table for eps (considered clear sky, I think). TBH: I'm not sure exactly what to do here, but semantically I still think that NaNs make more sense here than zeros or raising an exception.

More broadly, should this perez function be first checking the inputs to ensure that 0 <= dni < inf and 0 <= dhi < inf, but (possibly) allowing upstream NaNs to "pass through"?

cwhanse · 2026-07-07T16:50:47Z

@markcampanelli as a general rule (but not always), pvlib avoids checking inputs. GIGO applies.

Case e is an exception to the usual nan-in/nan-out philosophy, but it's been that way forever so I'm not inclined to suggest changing it. Case j is a variation of case e, so to be consistent, 0 output.

Case l is a problem that illustrates why the nan-in/nan-out rule has value. If pvlib outputs nan, then we'd do that by removing the overwriting airmass nan with 0 (Cases e and j). Since I'm not inclined to change that behavior, pvlib outputs 0 for case l. Otherwise we write some more complicated logic - ugh.

Case i: I'd prefer to output the negative numbers rather than substituting nan where the values are non-physical (this was likely the motivation for overwriting airmass nan). Outputting nan should be a clue that you've got nan inputs. Outputting negative irradiance is a different signal - you're using the model outside it's intended range.

Handle 0/0 in perez without NaNs

ae3b8ff

markcampanelli commented Jul 5, 2026

View reviewed changes

markcampanelli mentioned this pull request Jul 5, 2026

Keep or exclude total diffuse irradiance in the output when return_components=True #2801

Open

markcampanelli marked this pull request as draft July 5, 2026 18:04

markcampanelli added 9 commits July 5, 2026 13:27

Add return_components=True path to tests

8755ecc

Appease flake8

60bf26d

Add and improve tests

483c9b8

Make sure NaN irradiances pass through as expected

e97b83f

Improve NaN DNI and DHI division combo testing

303ae01

Push breaking test for discussion purposes

46970d2

Fix remaining expected values in broken test

6f6d44c

Update docstring for expected NaN airmass behavior

4f1b664

Do not overwrite approapriate NaNs with zero when airmass is NaN

f6c8070

markcampanelli commented Jul 5, 2026

View reviewed changes

markcampanelli added 2 commits July 5, 2026 17:23

Update some comments and whatsnew

2a2185f

Appease flake8 yet again

acaa15f

cwhanse reviewed Jul 6, 2026

View reviewed changes

markcampanelli and others added 3 commits July 7, 2026 07:59

Merge branch 'pvlib:main' into align_perez_with_perez_driesse

e22af42

Update docs/sphinx/source/whatsnew/v0.15.3.rst

b3b5d75

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Update docs/sphinx/source/whatsnew/v0.15.3.rst

cb26543

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Uh oh!

Conversation

markcampanelli commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcampanelli commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwhanse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcampanelli commented Jul 6, 2026 • edited by cwhanse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markcampanelli commented Jul 7, 2026

Uh oh!

cwhanse commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markcampanelli commented Jul 5, 2026 •

edited

Loading

markcampanelli commented Jul 5, 2026 •

edited

Loading

cwhanse left a comment •

edited

Loading

markcampanelli commented Jul 6, 2026 •

edited by cwhanse

Loading