DISC Knc out of range

[adriesse]

The expression for the DNI clearness index for clear conditions (Knc) is a polynomial fit over the air mass range 1 to 12. The values of this polynomial increase with air mass values > 17 and reach as high as 6 with air mass 37, which is clearly not realistic.

In the current DISC implementation air mass values up to 37 are allowed, and unrealistic DNI values appear when zenith > 88 / air mass > 19.

I suggest clipping zenith at 85 or 86 instead of clipping cos(zenith) to 0.065. This produces a more graceful transition to night time.

[wholmgren]

@adriesse I'm a bit confused by

In the current DISC implementation air mass values up to 37 are allowed, and unrealistic DNI values appear when zenith > 88 / air mass > 19.

pvlib currently sets DNI to 0 if zenith > 87:

dni = np.where((zenith > 87) | (ghi < 0) | (dni < 0), 0, dni)

See also #311, #318, and #400.

[adriesse]

I guess I was looking at an earlier version where that test had (zenith > 90) etc. Maybe I didn't pay attention to everything that was going on.

Still, I would prefer a solution limiting air mass going into the Knc polynomial. I think then DISC should work ok until zenith=90.

[wholmgren]

I forgot to mention that I added the following comment to _disc_kn when I refactored DISC to support GTI-DIRINT:

    # consider adding
    # am = np.maximum(am, 12)  # GH 450

I now realize that I got it wrong -- that should be np.minimum!

@cwhanse you added the 0.6.1 milestone to this issue. Does that mean that you support adding the airmass limit to _disc_kn?

[cwhanse]

@wholmgren @adriesse Yes, let's put a max_airmass kwarg with default of 12.

[adriesse]

Don't forget that _disc_kn is also used in gti_dirint...

I think I would prefer to see a hard-coded limit _disc_kn. All the coefficients are hard-coded anyway, and I can see no good reason to use this polynomial to extrapolate. Based on my opening observations, it might be ok to extrapolate to airmass 17, but definitely not further.

[wholmgren]

Don't forget that _disc_kn is also used in gti_dirint...

Thanks, I did forget this once I ran into an implementation issue. The issue is that disc returns am, and I thought that it should return the limited am. So, I moved the np.minimum call up to disc. Probably better to move the limit back to _disc_kn and let that function also return the modified am to calling functions.

I think I would prefer to see a hard-coded limit _disc_kn.

I also support a hard coded limit in _disc_kn. It would be easy to pass the new disc kwarg to _disc_kn, but that's just one more thing that can break in the future. I didn't want to go through the effort of exposing (and testing) this kwarg in dirint and dirindex because the added value seemed so small.

I guess another option is add the kwarg to _disc_kn but don't expose it in disc. Maybe this is actually what @cwhanse proposed and I misunderstood. I'm ok with this option too.