limit cos(aoi) in diffuse sky functions #535

wholmgren · 2018-08-14T17:09:23Z

Fixes two issues related to projection of the solar angle:

Dividing by near-0 cos(solar_zenith) when the sun is very close to the horizon.
non-0 contributions of the beam irradiance to the reindl, haydavies, and klucher functions.

Implementation is up for debate. Seemed easier to make a pull request with a concrete proposal.

I considered calculating the cos_solar_zenith as:

cos_solar_zenith = aoi_projection(0, 0, solar_zenith, 0, projection_minimum=0.01745)

but I am worried this could lead to issues with small floating point arithmetic (worse, they could be platform and python/numpy version dependent).

Closes Dividing by cos(solar_zenith) in haydavies and reindl functions #432, Closes Nonzero direct irradiance contribution to sky diffuse when sun is behind the array #526
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 git diff upstream/master -u -- "*.py" | flake8 --diff
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.

other:

Add test values for solar zenith between 89 and 90 deg.
Rerun irradiance tutorial

adriesse · 2018-08-14T19:47:41Z

pvlib/irradiance.py

+        Minimum allowable value for the projection for the solar angle
+        into the plane of the array. For example, 0 is equivalent to a
+        90 degree limit, 0.01745 is equivalent to a 89 degree limit.
+        `None` applies no limit.



Your explanation makes me wonder whether the minimum would be better expressed in degrees.

Yes, I also wondered this. I only went with the projection because it's the quantity that is hardcoded in the matlab library.

After thinking through the response to your comment below, I am leaning towards keeping it as the dot product to avoid the complexity of handling angles > 90 degrees or less than 0.

I'm having trouble seeing a use case which requires projection_minimum. The function's description says it calculates a dot product so that's what I'd expect it to do, and to not do more.

Actually it does less than a dot product since it assumes/requires the vectors are unit vectors. It's really just calculating the cosine of the angle of incidence. I think the name "projection" seems to suggest more, and may even be confusing to some.

I will remove the projection_minimum kwargs and put the limits in the diffuse models where necessary. We can address the function name in a separate issue.

adriesse · 2018-08-14T19:56:16Z

pvlib/irradiance.py


    cos_solar_zenith = tools.cosd(solar_zenith)

+    if solar_zenith_projection_minimum is not None:


I don't think None should be possible here.

I suppose we could go with a default value of -1 and always apply the limit.

adriesse · 2018-08-14T20:04:35Z

pvlib/irradiance.py


-    cos_solar_zenith = tools.cosd(solar_zenith)
+    cos_solar_zenith = np.maximum(tools.cosd(solar_zenith), 0.01745)


For Rb this limit might be good, but for HB further below the limit should be zero (already enforced).

Agreed. I'll fix this.

IMO it is more clear to leave the cos_tt and cos_solar_zenith calculations alone, and apply the filters to the value of Rb.

@cwhanse I figured you'd be in favor of being consistent with the matlab approach. I like it because it addresses both #432 and #526.

cwhanse · 2018-08-14T21:52:43Z

pvlib/irradiance.py

+        Minimum allowable value for the projection for the solar angle
+        into the plane of the array. For example, 0 is equivalent to a
+        90 degree limit, 0.01745 is equivalent to a 89 degree limit.
+        `None` applies no limit.



I'm having trouble seeing a use case which requires projection_minimum. The function's description says it calculates a dot product so that's what I'd expect it to do, and to not do more.

cwhanse · 2018-08-14T21:54:15Z

pvlib/irradiance.py

+        into the plane of the array. `None` applies no limit.
+    solar_zenith_projection_minimum : numeric, default None
+        Minimum allowable value for the projection of the solar angle
+        on to a horizontal surface. `None` applies no limit.


I'm struggling to see how the xxx_minimum kwargs are more clear than handling the behind-the-plane or nearly co-planar cases directly, in the calculation of the ratios in the diffuse sky models.

That's fair. I think this is a borderline case of "don't repeat yourself". I have no idea if people use these functions on their own.

cwhanse · 2018-08-15T14:39:42Z

pvlib/irradiance.py


    cos_solar_zenith = tools.cosd(solar_zenith)

+    cos_solar_zenith = np.maximum(cos_solar_zenith,
+                                  solar_zenith_projection_minimum)
+
    # ratio of titled and horizontal beam irradiance


Nit: tilted not titled

wholmgren · 2018-08-17T14:18:58Z

@cwhanse is it easy enough for you to run the matlab equivalents of klucher, haydavies, reindl, king with the same inputs as our test suite?

mikofski · 2018-08-17T15:56:06Z

@cwhanse is pvlib-matlab all functions, no classes? If so it should run in Octave, so then if there's a test suite, you could run it on a CI server like Travis. Just use sudo install octave in the build script

cwhanse · 2018-08-17T18:54:28Z

Yes, PVLib for Matlab is a collection of function. We currently have no test suite for PVLib for Matlab, which is a major deficiency.

wholmgren · 2018-08-18T17:15:48Z

I'm working on some matlab/python tests here: https://github.com/wholmgren/MATLAB_PV_LIB/blob/skydifftest/tests/test_sky_diffuse.m I have little idea about what I'm doing with matlab testing, but it seems to get the job done for this pvlib python PR. @cwhanse if you're interested I can make a PR to the matlab repository, but I imagine you all might have a preferred way to do the testing.

cwhanse · 2018-08-18T19:12:30Z

Klucher:

The expected result for test_klucher_series is the same as produced by PVLib for MATLAB. However, PVLib for MATLAB does not filter out for cos(AOI) < 0, which is why the expected result is different from the calculated results. Bug in PVLib for MATLAB, in my opinion.

cwhanse · 2018-08-18T19:23:57Z

pvlib/irradiance.py

@@ -836,8 +837,9 @@ def haydavies(surface_tilt, surface_azimuth, dhi, dni, dni_extra,
    if projection_ratio is None:
        cos_tt = aoi_projection(surface_tilt, surface_azimuth,
                                solar_zenith, solar_azimuth)
+        cos_tt = np.minimum(cos_tt, 0)  # GH 526


This should be np.maximum

Hay and Davies:

test_haydavies fails because of this error np.minimum. Otherwise I believe it should pass.

cwhanse · 2018-08-18T19:30:29Z

pvlib/irradiance.py

@@ -932,11 +934,13 @@ def reindl(surface_tilt, surface_azimuth, dhi, dni, ghi, dni_extra,

    cos_tt = aoi_projection(surface_tilt, surface_azimuth,
                            solar_zenith, solar_azimuth)
+    cos_tt = np.minimum(cos_tt, 0)  # GH 526


np.maximum here

wholmgren · 2018-08-19T00:21:09Z

The test failures are unrelated. I believe this is ready to merge.

limit cos(aoi) in diffuse sky functions

9a74404

wholmgren added bug api labels Aug 14, 2018

wholmgren added this to the 0.6.0 milestone Aug 14, 2018

adriesse reviewed Aug 14, 2018

View reviewed changes

no limit on cos_solar_zenith in hb calculation. change proj limit to -1

c8530a2

cwhanse reviewed Aug 14, 2018

View reviewed changes

cwhanse reviewed Aug 15, 2018

View reviewed changes

cwhanse approved these changes Aug 15, 2018

View reviewed changes

reimplement without projection_minimum kwarg

c0943a8

cwhanse closed this Aug 18, 2018

cwhanse reopened this Aug 18, 2018

cwhanse requested changes Aug 18, 2018

View reviewed changes

cwhanse reviewed Aug 18, 2018

View reviewed changes

wholmgren added 3 commits August 18, 2018 12:34

fix np.minimum vs. np.maximum

205cc16

update tutorial

1122055

add sky diffuse test for zenith close to 90

0e3fb1a

wholmgren added 2 commits August 18, 2018 17:41

Merge remote-tracking branch 'pvlib/master' into aoidiff

e23d9d3

fix botched whatsnew merge [skip ci]

eead186

cwhanse approved these changes Aug 19, 2018

View reviewed changes

cwhanse merged commit 4f3b7e4 into pvlib:master Aug 19, 2018

wholmgren deleted the aoidiff branch August 19, 2018 17:22

wholmgren mentioned this pull request Jan 12, 2019

Negative "POA horizontal ratio" when sun behind array #641

Closed

adriesse mentioned this pull request Jan 19, 2019

Expand docstring for aoi_projection. #648

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

limit cos(aoi) in diffuse sky functions #535

limit cos(aoi) in diffuse sky functions #535

wholmgren commented Aug 14, 2018 •

edited

Loading

adriesse Aug 14, 2018

wholmgren Aug 14, 2018

wholmgren Aug 14, 2018

cwhanse Aug 14, 2018

adriesse Aug 15, 2018

wholmgren Aug 15, 2018

adriesse Aug 14, 2018

wholmgren Aug 14, 2018 •

edited

Loading

adriesse Aug 14, 2018

wholmgren Aug 14, 2018

cwhanse Aug 14, 2018

wholmgren Aug 14, 2018

cwhanse Aug 14, 2018

cwhanse Aug 14, 2018

wholmgren Aug 14, 2018

cwhanse Aug 15, 2018

wholmgren commented Aug 17, 2018

mikofski commented Aug 17, 2018

cwhanse commented Aug 17, 2018

wholmgren commented Aug 18, 2018

cwhanse commented Aug 18, 2018

cwhanse Aug 18, 2018

cwhanse Aug 18, 2018

cwhanse Aug 18, 2018

wholmgren commented Aug 19, 2018


		cos_solar_zenith = tools.cosd(solar_zenith)

		if solar_zenith_projection_minimum is not None:


		cos_solar_zenith = tools.cosd(solar_zenith)
		cos_solar_zenith = np.maximum(tools.cosd(solar_zenith), 0.01745)

limit cos(aoi) in diffuse sky functions #535

limit cos(aoi) in diffuse sky functions #535

Conversation

wholmgren commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Aug 17, 2018

mikofski commented Aug 17, 2018

cwhanse commented Aug 17, 2018

wholmgren commented Aug 18, 2018

cwhanse commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented Aug 19, 2018

wholmgren commented Aug 14, 2018 •

edited

Loading

wholmgren Aug 14, 2018 •

edited

Loading