Update irradiance.aoi to use more reliable formula #1191

kandersolar · 2021-03-10T01:05:50Z

Closes irradiance.aoi can return NaN when module orientation is perfectly aligned with solar position #1185
I am familiar with the contributing guidelines
Tests added
~~Updates entries to docs/sphinx/source/api.rst for API changes.~~
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
~~New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.~~
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

I decided the proposed option 2 in #1185 made the most sense, but happy to go another way if a better fix exists.

cwhanse · 2021-03-10T05:04:33Z

I don't think option 4 is feasible. There will always be some combination of inputs that produce a round off error.

My only thought about using np.clip is, what if somehow in some really weird way tools.cosd (an untested pvlib function) changes and fails to produce values in [-1, 1] within roundoff? We wouldn't know.

An alternative is to truncate projection to 15 decimal places, since the round off surely won't be greater than the minimum floating point number (about 2.2e-16). In the example you found, that is exactly the amount that projection exceeds 1. I think this is Option 1.

In any case, I think the risk is small with the simple np.clip in this PR, and will not object if this goes forward.

Or, use np.where in a sort of complicated way, to replace (projection > 1) & (projection <= 1.0 + eps) with 1.0.

cwhanse · 2021-03-15T15:15:27Z

pvlib/irradiance.py


    Input all angles in degrees.

+    .. versionchanged:: 0.9.0


I didn't know about this directive. Maybe we could be using it more often.

cwhanse · 2021-03-15T15:19:45Z

pvlib/irradiance.py

+    aoi = 2 * np.arctan(c / np.sqrt((1+(1+c))*((1-c)+1)))
+    aoi_value = np.rad2deg(aoi)
+
+    for arg in [surface_tilt, surface_azimuth, solar_zenith, solar_azimuth]:


I think it's OK to assume that these Series have a common index, and stop the loop when one index if found.

does the new implementation preserve Series in --> Series out without this code?

I'm fine with dropping the name assignment and letting users deal with that.

does the new implementation preserve Series in --> Series out without this code?

Unfortunately it does not; the _spherical_to_cartesian function turns Series into 2-D numpy arrays. I don't immediately see a way around it that's cleaner than the "fix types at the end" idiom.

cwhanse · 2021-03-15T15:22:04Z

pvlib/irradiance.py


    Input all angles in degrees.

+    .. warning::


I would not be opposed to retiring the aoi_projection function. aoi could return cos(aoi) also, if that's of interest.

+1 on retiring aoi_projection, but let the few users that need cos(aoi) call it themselves.

It's not a big deal, but several of the sky diffuse transposition models use aoi_projection(), so that deprecation will need some other minor changes too.

-1 on retiring aoi_projection because I use it.

thanks @adriesse. I suggest we keep it and clip the output. Perhaps we should document the formula used by each function.

wholmgren · 2021-03-15T17:23:25Z

pvlib/irradiance.py

+    aoi = 2 * np.arctan(c / np.sqrt((1+(1+c))*((1-c)+1)))
+    aoi_value = np.rad2deg(aoi)
+
+    for arg in [surface_tilt, surface_azimuth, solar_zenith, solar_azimuth]:


does the new implementation preserve Series in --> Series out without this code?

I'm fine with dropping the name assignment and letting users deal with that.

wholmgren · 2021-03-15T17:25:16Z

pvlib/irradiance.py


    Input all angles in degrees.

+    .. warning::


+1 on retiring aoi_projection, but let the few users that need cos(aoi) call it themselves.

adriesse · 2021-03-19T09:14:21Z

Further to my request to keep the function, I searched my code base and I make relatively few uses of it, mainly in code where I did not want a dependency on my private toolbox.

As to my usage patterns:

I don't always need aoi when I need cosinc (I like short names)
But I always need to clip cosinc to positive numbers
As a result I usually first calculate cosinc, then aoi just in case I need it, then clip cosinc to positive

wholmgren · 2021-04-21T03:33:57Z

Should we merge this as is? Should we also add clip to the aoi projection function? Should we forget about the clever math and just use clip? I'm ok with any of these options.

adriesse · 2021-04-21T08:55:11Z

pvlib/irradiance.py

+    .. versionchanged:: 0.9.0
+       Updated to a slower but more reliable formula that correctly handles
+       cases where the surface normal and solar position vectors are parallel.
+


This method treats aforementioned vectors as sides of a triangle, a and b, each length one. The value c in the code below is the length of the third side.

adriesse · 2021-04-21T08:55:16Z

pvlib/irradiance.py

-        aoi_value.name = 'aoi'
-    except AttributeError:
-        pass
+    aoi = 2 * np.arctan(c / np.sqrt((1+(1+c))*((1-c)+1)))


I think since a and b both equal 1, the sequencing of operations is not necessary, and I would suggest:
np.arctan2(c, np.sqrt((2.0 + c) * (2.0 - c)))

adriesse · 2021-04-21T10:06:45Z

After finally following the links and looking at the reference, I must say I found these thoroughly interesting!

However, I am not convinced that a triangle formed with two unit vectors runs as many risks of numerical issues as and arbitrary triangle with sides a, b and c, and forming the vectors in the first place requires substantially more imprecise trig function calls. Furthermore, this method produces the aoi directly, so for most purposes a cos() call must follow.

I vote for a few well-placed clips to keep values sane.

kandersolar · 2021-05-14T01:39:32Z

I vote for a few well-placed clips to keep values sane.

This PR started out with a single clip near the end of the aoi_projection function -- anyone object to going back to 8069470? I mildly prefer it over maintaining an arcane equation I only sort of understand.

wholmgren · 2021-05-14T02:17:17Z

No objection.

adriesse · 2021-05-14T07:29:10Z

No objection. It was nice to learn about this other formula.

wholmgren · 2021-05-14T15:35:56Z

pvlib/tests/test_irradiance.py

+    zenith = 89.26778228223463
+    azimuth = 60.932028605997004
+    projection = irradiance.aoi_projection(zenith, azimuth, zenith, azimuth)
+    assert projection == 1


I don't know enough to intelligently comment on this, but I noticed that we're clipping to the int values -1 and 1. Is projection here also an int? I believe that if tested with realistic array input we'd always see floats returned. Might be worth testing that the clip behavior remains as expected in that case. Perhaps the important thing is that projection is always <= 1 and that np.isclose(projection, 1) returns True.

Good suggestions. I also added a dtype check to make sure that clipping with int limits doesn't do something funky.

cwhanse · 2021-05-14T17:11:23Z

Lets merge

kandersolar added 3 commits March 9, 2021 18:01

clip to [-1, 1]

8069470

add test

e0e7d99

whatsnew

2a0425b

kandersolar added the bug label Mar 10, 2021

kandersolar added this to the 0.9.0 milestone Mar 10, 2021

kandersolar added 3 commits March 13, 2021 14:17

use new aoi formula

3ee6418

revert changes to aoi_projection

8fb5070

update test and whatsnew

30cb9f2

kandersolar changed the title ~~Clamp AOI projection values to [-1, +1]~~ Update irradiance.aoi to use more reliable formula Mar 13, 2021

cwhanse reviewed Mar 15, 2021

View reviewed changes

cwhanse approved these changes Mar 15, 2021

View reviewed changes

wholmgren approved these changes Mar 15, 2021

View reviewed changes

don't set name; break after first Series

671cf16

adriesse reviewed Apr 21, 2021

View reviewed changes

cwhanse mentioned this pull request May 5, 2021

pvlib v0.9 release planning #1214

Closed

24 tasks

kandersolar added 2 commits May 14, 2021 09:06

roll back to 2a0425b

c2e6471

Merge remote-tracking branch 'upstream/master' into aoi_projection_clamp

669483d

wholmgren reviewed May 14, 2021

View reviewed changes

test improvements

ab7b57d

wholmgren approved these changes May 14, 2021

View reviewed changes

wholmgren merged commit 5bdad64 into pvlib:master May 14, 2021

kandersolar deleted the aoi_projection_clamp branch May 14, 2021 17:14

kandersolar mentioned this pull request Apr 28, 2022

Bump pandas to 0.25.0; test updates #1448

Merged

7 tasks

Update irradiance.aoi to use more reliable formula #1191

Update irradiance.aoi to use more reliable formula #1191

Uh oh!

Conversation

kandersolar commented Mar 10, 2021

Uh oh!

cwhanse commented Mar 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriesse commented Mar 19, 2021

Uh oh!

wholmgren commented Apr 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriesse commented Apr 21, 2021

Uh oh!

kandersolar commented May 14, 2021

Uh oh!

wholmgren commented May 14, 2021

Uh oh!

adriesse commented May 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwhanse commented May 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants