Add irradiance.clearsky_index

[kevinsa5]

• Closes issue Add clearsky index function to pvlib.irradiance #551
• I am familiar with the contributing guidelines.
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
• 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.
• Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
• New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
• Pull request is nearly complete and ready for detailed review.

[kevinsa5]

Two questions for the pvlib team:

• Anyone have a citation for the definition of the clear-sky index? My searching was unsuccessful.
• I don't have much experience writing tests for numerical functions -- is the approach used in the tests for irradiance.clearness_index a good example to follow? Hardcoding numbers strikes me as bad form, but the alternative would just be to reimplement the function in the test...

[wholmgren]

Thanks @kevinsa5!

I recommend limiting this PR to only clearsky_index.

Copying and modifying test_irradiance.test_clearness_index is a good approach. Hardcoding numbers is simple and easy to understand. I think it's an ok approach for the basic functions. We take a different approach for the wrapper methods.

[cwhanse]

@kevinsa5 also, thanks. I'd recommend hard-coding some inputs you select, and not running a clear-sky model to generate inputs for the test.

[stickler-ci]

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

[codecov]

Codecov Report

Merging #571 into master will decrease coverage by 72.83%.
The diff coverage is 11.11%.

@@             Coverage Diff             @@
##           master     #571       +/-   ##
===========================================
- Coverage   93.31%   20.48%   -72.84%     
===========================================
  Files          42       42               
  Lines        6916     6941       +25     
===========================================
- Hits         6454     1422     -5032     
- Misses        462     5519     +5057

Impacted Files	Coverage Δ
pvlib/test/test_irradiance.py	18.32% <10.52%> (-81.42%)	⬇️
pvlib/irradiance.py	9.84% <12.5%> (-85.87%)	⬇️
pvlib/test/test_location.py	9.03% <0%> (-89.76%)	⬇️
pvlib/test/test_singlediode.py	14.73% <0%> (-85.27%)	⬇️
pvlib/test/test_solarposition.py	12.5% <0%> (-85.07%)	⬇️
pvlib/test/test_tracking.py	15.65% <0%> (-84.35%)	⬇️
pvlib/iotools/srml.py	15.9% <0%> (-84.1%)	⬇️
pvlib/test/test_pvsystem.py	15.35% <0%> (-83.16%)	⬇️
pvlib/tracking.py	17.47% <0%> (-82.53%)	⬇️
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 673ab97...4a215f5. Read the comment docs.

[stickler-ci]

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

[stickler-ci]

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

[kevinsa5]

Apologies for the long delay on this PR. Two questions:

• I'm not very experienced in CI, but it seems like the travis failures are related to the test framework, rather than the pvlib code?
• Also unfamiliar with this stickler bot. Shall I add the .stickler.tml from the master branch to this PR?

[wholmgren]

@kevinsa5 there are new test failures due to a new version of pytest and there is a known failure due to python/numba. Just make sure your tests are passing.

Can you merge the latest pvlib/master into your branch? That should allow stickler to check your code for style/formatting issues.

[cwhanse]

@kevinsa5 we'd like to merge this before v0.6.1 release. Could you go through the pull request checklist? I think you'll only need to add entries to api.rst and whatsnew. Thanks again.

[wholmgren]

this looks close, but what's the time frame for finishing this? there will always be a new feature to add and at some point we need to draw a line and cut the 0.6.1 release.

[kevinsa5]

I appreciate everyone's patience here. If there are any other outstanding issues keeping this from being merge-ready, please let me know.

[wholmgren]

Looks good to me so long as we change the PR and issue names to something about clearsky index instead of the more generic "irradiance metrics".

[wholmgren]

Looks good to me. Thanks @kevinsa5 for your contribution and patience with the review.

@cwhanse merge if you approve.

[cwhanse]

@kevinsa5 thanks again!

[adriesse]

Please let me know whether I understand the code correctly:

• clearsky_index = ghi / clearsky_ghi propagates nans without creating new ones.
• clearsky_index = np.maximum(clearsky_index, 0) pulls -inf to zero
• clearsky_index = np.minimum(clearsky_index, max_clearsky_index) pulls +inf to max_clearsky_index

So the extra steps basically serve to pull +inf down to zero rather than to max_clearsky_index?

[kevinsa5]

clearsky_index = ghi / clearsky_ghi will both propagate nans and create new nans -- consider ghi = clearsky_ghi = 0. The extra steps are so input nan/inf are mapped to nan, but any nan/inf created internally result in zero, eg:

In [9]: clearsky_index(np.array([1,1,np.nan]), np.array([np.nan, 0, 1]))
Out[9]: array([ nan,   0.,  nan])

[adriesse]

Thanks!