pvlib.irradiance._calc_delta documentation/usage

[markcampanelli]

The docstring for pvlib.irradiance._calc_delta currently states:

Compute the delta parameter, which represents sky dome "brightness" in the Perez and Perez-Driesse models.

This is somewhat misleading, because this function is not currently used In the pvlib.irradiance.perez function. Perhaps it should be? If not, then I suggest updating the language. (_calc_zeta's docstring notably only mentions Perez-Driesse.)

In addition, after a helpful communication with @adriesse, it seems that the inaugural pvlib.irradiance.perez function does not indicate in its doctoring that kastenyoung1989 is (apparently) the preferred method for computing the airmass parameter.

Was there ever a discussion about aligning the airmass input, its (default) calculation, and the delta calculation between pvlib.irradiance.perez and pvlib.irradiance.perez_driesse?

(Same question could be made for zeta and _calc_zeta usage, I suppose.)

I have been somewhat frustrated by the divergence in behavior of perez and perez-driesse, with the latter seeming to have better safeguards in place. This was much of the impetus behind #2808.

[cwhanse]

@markcampanelli can you clarify a bit what you see as the problem(s) and what you suggest as the solution(s)?

[markcampanelli]

@markcampanelli can you clarify a bit what you see as the problem(s) and what you suggest as the solution(s)?

@cwhanse Please see this recent commit for a choice that tries to better align perez and perez_driesse inputs and functionality. Of course, a bunch of tests break, and I'd have to sort through and justify+fix all the differences.

[markcampanelli]

I think some of test breakages may have to do with changes between large air masses being bounded as a real number vs. being NaN, but I'd have to dig through the details to verify. Not sure when I'll have more time for that.

[cwhanse]

So basically, you are proposing that

1. irradiance.perez uses the _calc_delta and _calc_zeta helper function that was added for irradiance.perez_driesse
2. irradiance.perez uses the _calc_zeta helper function that was added for irradiance.perez_driesse
3. irradiance.perez defaults to calculating airmass with kastenyoung if airmass is not provided, which implies that airmass becomes an optional rather than required argment.

#1 may make sense, _calc_delta adds some handling of 0 / NaN values.
#2 is a change in the underlying regressions (I think) so the result would no longer be the model that Perez published. If that's so, then #2 is a problem.
#3 is a change in behavior, that is not a significant problem because airmass is fortuitously the last argument before named kwargs. I'm not necessarily opposed, but maybe we can handle the ambiguity by specifying the airmass model in the documentation.

I'm still not clear what problems these changes will fix, or what is being gained by these changes.

[markcampanelli]

@cwhanse Thanks for reviewing the proposed changes. Incidentally, I am not a huge fan of default values in Python, because removing or changing them then becomes a breaking change. I'd probably prefer newly accepting None for airmass while not making it the default.

I'm still not clear what problems these changes will fix, or what is being gained by these changes.

The Notes for perez_driesse state:

The Perez-Driesse model can be considered a plug-in replacement for the
1990 Perez model using the 'allsitescomposite1990' coefficient set.
Deviations between the two are very small, as demonstrated in [1]_.

Perhaps "plug-in replacement" does not refer to the code implementation in pvlib? I almost always have to change my code when switching between perez and perez_driesse. Granted, these seem to be mostly for edge cases, but they are common enough that it still screws up results and (IMHO) reduces confidence in the library.

[markcampanelli]

Update here on the differences in relative airmass calculations:

In perez_driesse makes an adjustment to the (relative) airmass, be it a non-None airmass passed in by the user or returned internally by pvlib.atmosphere.get_relative_airmass (when airmass=None). Specifically, _calc_delta applies a large, but bounded, max value to airmass when the zenith is at/over 90 degrees:

max_airmass = atmosphere.get_relative_airmass(90, 'kastenyoung1989')
airmass = np.where(solar_zenith >= 90, max_airmass, airmass)

whereas get_relative_airmass normally returns NaN where solar_zenith >= 90.

I wonder how many perez test failures are due to this difference. I also wonder if that maximum airmass is specified anywhere in the literature, which @adriesse may recall. Will try to figure these out as time permits.

[adriesse]

A few comments on the above discussion:

• "plug-in replacement" is a quote from the paper pertaining to the model rather than the implementation. We did try to implement it in the same spirit by keeping almost the same signature, but did not back-port any improvements, like a cap on F1 for example.
• The phrase "(careful using the 1/cos(z) model of AM generation)" has always bothered me because it seems to imply that it is ok to use this model. We probably should have dropped that phrase and put in a comment about KastenYoung instead. And we should have back-ported that.
• The limits on the inputs to many models are not discussed in the literature and so we happily extrapolate beyond them in many cases. I think it is our responsibility to deal with limits in a reasonable manner in the implementations. Using Perez for zenith angle > 90 is extrapolating.
• It does not make much sense to calculate the path through the atmosphere of sun rays when there are no sun rays, so in some contexts a value of nan seems logical and prevents nonsensical extrapolation of air mass values. Air mass is actually an intermediate variable in the Perez models. In this context, such nans are unhelpful.
• If I understand correctly, you are mostly unhappy about non-matching nans. From memory, I think I did not want nans in tilted irradiance when my inputs had zero irradiance. I still don't.
• If you want to update perez() to use helper functions, I suggest making a new _calc_epsilon()

Finally, I do think it is a very good idea to align the implementations more!