Add GTI DIRINT model

[wholmgren]

This PR adds an implementation of the GTI DIRINT model.

I'm not confident about my API design/changes, but I'm making the PR now due to new interest in issue #396. See the whatsnew diff for a list of the changes to existing functions. I did this to avoid copy/pasting code, but maybe there's a better way. Let me know what you think.

The algorithm works well for angles less than 90 degrees. It struggles for angles greater than 90 degrees. I'm not sure if that's because I made a mistake. I've only used synthetic data to test it so far.
Edit: it now seems to work ok for angles greater than 90 degrees and it struggles for angles within a few degrees of 90.

Here's a notebook with examples:

https://gist.github.com/wholmgren/fe4d72ebb2c0e6481680bee5d9041b06

• Closes Add GTI DIRINT model #396. Closes irradiance.disc has unbounded clearness index calculation #540
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
• Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff and/or landscape.io linting service.
• New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
• Updates entries to docs/sphinx/source/api.rst for API changes.
• Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.

other:

• Fix AOI < 90 case or add comment regarding performance
• Remove debug kwarg
• tests for new kwargs in disc and dirint
• tests for clearness index functions
• fix python 2.7 min failure
• decide zero or nan and remove zero_instead_of_nan block
• remove tutorial

[wholmgren]

And thanks to @cwhanse for reviewing a previous version of this code and helping me figure out some problems with the implementation.

[jforbess]

For what it's worth, once I figured out what you did to add the params to the disc and dirint functions, I thought it was a good idea. I used it fairly successfully, but didn't test it strongly myself, because I didn't have measured GHI (thus the exercise).

…

On Thu, Nov 30, 2017 at 12:14 PM, Will Holmgren ***@***.***> wrote: And thanks to @cwhanse <https://github.com/cwhanse> for reviewing a previous version of this code and helping me figure out some problems with the implementation. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#400 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH66AfvQeeXQIHGKbtDzwGJEBBUlwprSks5s7wy4gaJpZM4QxIdl> .

[adriesse]

Hi Will,
Would you give me a hint on the git incantations I need to invoke to try this new code before it gets merged or released?
Thanks!

[wholmgren]

Sure. I'd do it like this on the command line (adapted from #443):

1. add a new "remote" so you can get my versions of pvlib-python on github. This will associate the name "wholmgren" with the url, but does nothing more: git remote add wholmgren https://github.com/wholmgren/pvlib-python.git
2. fetch the data from the wholmgren remote (this doesn't touch your data or any local changes you've made. it just stores it in a hidden directory): git fetch wholmgren
3. make a new local branch that's based on wholmgren/gtidirint: git checkout -b gtidirint wholmgren/gtidirint

I have no idea if the GitHub desktop client or similar can do this kind of thing. Maybe others can comment if they have experience.

Alternatively, you can skip git entirely by navigating to my gtidirint branch on github and downloading a zip file of the code (green button at right): https://github.com/wholmgren/pvlib-python/tree/gtidirint

[adriesse]

Thanks! The command line version worked.
I got it to run also, however it appears the doc string needs updating.

[adriesse]

Would it be a good idea to convert any remaining 999 to nan at the end?

[wholmgren]

to convert the ghi, dni, dhi to nan where diff remains 9999? alternatively, we could do this for any point that does not converge. This seems to easily occur near aoi = 90.

[adriesse]

Parameters need updating.

[wholmgren]

updated.

[cwhanse]

I've looked into the source of 0.82 as the upper bound. It does not appear in the references, which don't specify an upper bound. The value 0.82 comes from a Fortran implementation of the DIRINT model at NREL, and appears to have been added as a modification to the original algorithm.

I recommend we keep the upper bound, but make it a kwarg (like min_cos_zenith) with default of 0.82, and point at NREL SRRL's Fortran code for DIRINT as the source (although the file appears no longer available through their website.)

An upper bound makes sense, but with the caution that the value will depend on the time averaging. 0.82 is probably an hourly value. For a minute, clearness index > 1 is certainly possible if the GHI is enhanced by cloud effects.

[wholmgren]

Thanks for the good comment. We should definitely make a kwarg. I'd recommend a maximum value of 2 for a few reasons. My general feeling is that the defaults should be as permissive as possible and the user has some responsibility to QA/QC their data before putting it into this function. Also, subhourly data seems to be common these days.

[cwhanse]

2 is OK with me. G. Yordanov has a few conference papers looking at the magnitude, duration and frequency of occurrence of over-irradiance, down to sub-second responses from a photodiode sensor. From memory, a maximum value of 2 would be a generous limit. 2 would likely also prevent the overflow warning reported in #540

[wholmgren]

I will add the kwarg to clearness_index and clearness_index_zenith_independent with default values of 2. Should it also be a kwarg in disc, dirint, dirindex (like min_cos_zenith and max_zenith)?

[cwhanse]

I think that those models should call clearness_index so if I understand rightly we’d need to add the kwarg to those functions. The bloating argument list is a little distressing.

[cwhanse]

Right, they do with this pull request. I think that disc and dirint should enforce a limit on kt. dirindex should The challenge here is that the limit is up to us. The only precedent we have is old SRRL Fortran code for dirint which is where 0.82 comes from. I'm fairly sure that value is appropriate for hourly data but not for shorter time steps. I agree with you that kt<2 is generally applicable and is better than 0.82.

I'd say let's go ahead and expose the kwarg.

[wholmgren]

Updating the unit tests has me thinking that within disc, dirint, dirindex we should enforce a limit of 0.82 or 1 or we should return NaN.

For disc, the input conditions that yield kt > 1.25 will produce dni = 0. This because Kn becomes negative and we set negative values to 0.

For dirint, the input conditions that yield kt_prime > 1 will produce dni = NaN.

Here's the line in dirint that currently limits kt_prime to 0.82:

https://github.com/pvlib/pvlib-python/blob/master/pvlib/irradiance.py#L1358

These lines assign bins for kt_prime values [0, 1]:

https://github.com/pvlib/pvlib-python/blob/master/pvlib/irradiance.py#L1381-L1387

These lines will ensure that kt_prime values > 1 will become NaN:

https://github.com/pvlib/pvlib-python/blob/master/pvlib/irradiance.py#L1425-L1428

One could argue that returning NaN is the right thing to do because the authors did not anticipate the model being applied for clearness index > 1. Or one could argue that we should limit the clearness index to 0.82 or 1 and move on.

[cwhanse]

I think you are suggesting that the call to clearness_index in, e.g., disc, passes a value max_kt=1 (or otherwise) that is appropriate for disc. If this is correct, then I'm OK. If you are suggesting that the default value for max_kt is set to prevent wonky output from disc then I'm not OK.

The appropriate value for each model is less clear. The only source we have is the SRRL code for dirint which is an uncommented addition to the original program. Since I am confident that none of these models meant to predict DNI from GHI during over-irradiance conditions, an upper bound of 0.82, or 1, seems appropriate to me.

On the other hand, for clearness_index itself, I like your suggestion of max_kt=2, for physical reasons.

I don't think disc etc. should return NaN unless its inputs are meaningless, or in the case of dirint are outside the model's domain of bins.

[wholmgren]

Sorry for being unclear. We agree (I think): within disc and dirint, we should call clearness_index and clearness_index_zenith_independent with max_clearness_index=1. The functions retain the default max value of 2. I'll update the PR this morning.

[wholmgren]

I changed the max clearness index values as discussed above.

[cwhanse]

Recommend making maximum of kt in clearness_index a kwarg with default 0.82. I put a comment at the appropriate point in the code.

[adriesse]

Just a general thought here triggered by the discussion around 0.82. It would be nice in this case and others to have a quick way to set up a model to run the way the original author proposed it, although we may all agree it is not the best way for today's toolbox. How about an optional kwarg settings_from which could have the name of a paper or institute or author and provide all the relevant settings?

[cwhanse]

@adriesse that's an idea worth pursuing. Maybe not in this particular pull request, but something to keep in mind if we have a model that has many 'settings' arising from ambiguous or conflicting information in the references.

I could get really excited about a settings_from method at the ModelChain level. Suppose we could init a ModelChain object with settings_from=<PAN file name> or some similar concept? The settings_from would import model selections, kwarg values, etc. that have been saved, and would go a long way towards promoting reproducibility.

[wholmgren]

@adriesse this sounds like #433. I'm not sure what it would mean in this specific case, though. Unbounded output?

[adriesse]

I guess the ideas are related. It would be nice to be able to reproduce original results without too much trouble as well as raise awareness when uncharted territory is entered.

[wholmgren]

I removed the gti_dirint.ipynb tutorial from this pull request and put it in this gist. The latest version explores why the gti_dirint function seems to yield bad results for AOI > 80 (roughly speaking) in some conditions. In short, the iterative algorithm is doing what it's supposed to do: minimize the difference between the modeled and measured POA global. However, this can lead to a less accurate modeled GHI/DNI/DHI at higher AOI. Refactoring the code has made me more confident in the implementation, but that doesn't guarantee it's correct.

[wholmgren]

See link below for rendered docs for new functions:

https://wholmgren-pvlib-python-new.readthedocs.io/en/gtidirint/api.html#irradiance

(scroll to bottom of the Irradiance section).

My recommendation is to merge it, though I am happy to take further suggestions if there are any.

[mikofski]

please use :doi:`10.1016/j.solener.2015.10.024`

[wholmgren]

thanks. updated.