search

pvlib / twoaxistracking

twoaxistracking is a python package for simulating two-axis tracking solar collectors, particularly self-shading.
https://twoaxistracking.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Correct nan handling in max_shading_elevation #28

Closed AdamRJensen closed 2 years ago

AdamRJensen commented 2 years ago

This PR fixes a bug in the way the max_shading_elevation function handles nan values, which can occur for very high ground coverage ratios (which is why it was able to go undetected).

The function essentially calculates the maximum shading elevation for each neighboring collector assuming the worst possible azimuth alignment. It does this calculation for both a circular and rectangular bounding box around the gross collector area. The result is then two arrays of the maximum shading elevation calculated for each neighboring collector. If the GCR is very high. Either the array of maximum shading elevation for the rectangular or circular case may yield nan values. The code previously ignored these nan values, which was incorrect, instead they should be set to 90 degrees. The final step of the calculation procedure is to take the maximum value of each array of values for the rectangular and circular cases. The actual maximum shading elevation can then be computed as the minimum of these two values. The concept may better be explained by the code below.

Original code (buggy):

max_elevation = np.nanmin([np.nanmax(max_elevations_rectangular),
                                               np.nanmax(max_elevations_circular)])

New code (correct):

max_elevation = np.min(
    [np.nan_to_num(max_elevations_rectangular, nan=90).max(),
     np.nan_to_num(max_elevations_circular, nan=90).max()])

Alternative solution letting the nans pass on to the minimum function (also correct):

max_elevation = np.nanmin([max_elevations_rectangular.max(),
                           max_elevations_circular.max()])

I found this mistake when I was running some simulations and got different results when I used the maximum shaded elevation calculated by max_shading_elevation vs. setting it to 90 degrees. Specifying the max shaded elevation should NOT have any impact on the shading results (that is if it is specified correctly); its only function is to skip simulations that do not have any shading.

When I corrected the nan handling in the max_shading_elevation function, it resulted in one test failure. I, therefore, calculated the maximum shading elevation by brute force for the specific test case and found that it was incorrectly set - so the test failure was a sign that the bug fix actually works.

kandersolar commented 2 years ago

Do you plan to tag a new release after merging this? Add the date to the what'snew page if so

AdamRJensen commented 2 years ago

Do you plan to tag a new release after merging this? Add the date to the what'snew page if so

I plan on adding a notebook on the validation since a section on this has been requested by one of the reviewers of the MethodsX paper. I guess it makes more sense to tag a release after this, which shouldn't be more than a day or two.