Tentative fix for #125 #128
Conversation
There was a problem hiding this comment.
wow thanks so much for catching this @kanderso-nrel 🙏 🚀
I gotta admit I'm not sure to understand the distinction between +0.0 and -0.0 in python so I'll have to look more into that...
I think this is a great fix and I would like to get it merged because the bug is serious IMO; and merging now doesn't prevent us from circling back to it at a later time.
| out = pvarray.ts_pvrows[1].back.get_param_weighted('qinc') | ||
| assert np.all(pvarray.ts_vf_matrix >= 0) | ||
| assert np.all(pvarray.ts_vf_matrix <= 1) | ||
| assert rear_qinc_expected == pytest.approx(out[0], abs=1e-2) |
pvfactors/geometry/base.py
Outdated
| @@ -122,7 +122,7 @@ def _get_rotation_from_tilt_azimuth(surface_azimuth, axis_azimuth, tilt): | |||
| # Calculate rotation of PV row (signed tilt angle) | |||
| is_pointing_right = ((surface_azimuth - axis_azimuth) % 360.) > 180. | |||
| rotation = np.where(is_pointing_right, tilt, -tilt) | |||
|
|
|||
| rotation = np.where(tilt == 0, -0.0, rotation) # GH 125 | |||
There was a problem hiding this comment.
wow I have no idea what's going on here 😅 but your test definitely shows that it's working so great catch! 🚀
There was a problem hiding this comment.
@kanderso-nrel I don't know what array sizes we are dealing with here, but this is likely faster and more memory efficient:
rotation[tilt==0] = -0.0
|
@campanelli-sunpower please feel free to add you review as well, otherwise I'd appreciate it if you could merge this PR |
There was a problem hiding this comment.
Thanks for submitting this tentative fix. It would be nice to know where exactly the sign on zero makes a difference. Do we leave #125 open for a more permanent fix?
pvfactors/geometry/base.py
Outdated
| @@ -122,7 +122,7 @@ def _get_rotation_from_tilt_azimuth(surface_azimuth, axis_azimuth, tilt): | |||
| # Calculate rotation of PV row (signed tilt angle) | |||
| is_pointing_right = ((surface_azimuth - axis_azimuth) % 360.) > 180. | |||
| rotation = np.where(is_pointing_right, tilt, -tilt) | |||
|
|
|||
| rotation = np.where(tilt == 0, -0.0, rotation) # GH 125 | |||
There was a problem hiding this comment.
@kanderso-nrel I don't know what array sizes we are dealing with here, but this is likely faster and more memory efficient:
rotation[tilt==0] = -0.0
|
Yeah, +1 from me to leave #125 open or at least close it and open a new issue for the follow-up. |
There was a problem hiding this comment.
I will leave #125 open with a note about this tentative fix. Thanks.
Playing around with more test cases like the ones in #125, I noticed a relationship between the discontinuity and the sign of the zero in
pvarray.rotation_vec(recall that+0.0and-0.0are distinct floating point values). The sign of the zero depends on the relationship betweensurface_azimuthandaxis_azimuth. I did not trace the code to understand what exactly changes downstream based on that sign, but standardizing the sign appears to resolve the problem -- no more negative viewfactors and no more irradiance discontinuity.I'm a little hesitant to consider this a proper fix without a full understanding of why it fixes the problem. Curious what others think!