-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
r.sun: Incorrect beam radiation for some steep inclined surfaces #2534
r.sun: Incorrect beam radiation for some steep inclined surfaces #2534
Conversation
…p approx north facing (in northern hemisphere) inclined surfaces is included in direct beam results. This was caused by an incorrect value of sunSlopeGeom->longit_l in the rsunlib.c, lumcline2 function. In main.c, in the calculate() function at line 1953, the value of q1 changes sign as the slope increases for north (in northern hemisphere) facing surfaces. This causes tan_lam_l to also change sign and hence the result of atan stored in sunSlopeGeom.longit_l to jump from just less than pi/2 to just more than -pi/2. The required value is pi more than this (which is another correct atan result as the tan plot repeats every pi radians). The fix detects this situation and adds pi to the result. I've done this addition in the rsunlib.c, lumcline2 function, as this is where the time offset is used. Also, there is a possibility of a divide by zero error if q1 is zero. This fix also corrects this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I can not fully judge the reasoning, code seems to be fine.
Although it is not mandatory, could you try to add a simple test case? You can take a look at r.in.pdal tests to see how to import small artificial DEM and how to compare generated output to a reference raster.
Well, re the test case using an artificial raster I did look at doing that, and also a C test aka lib/raster3d. The first was a bit far from the actual code but could work for the first fix, but the divide by zero will be messy due to rounding, and the second was going need more re-structuring of the code in r.sun than seemed reasonable. So I ended up writing some unit tests in Java against a cut & pasted copy of the lines I changed which at least gave me confidence, not appropriate for automated tests here though. |
I think comparing To help with the review, a simple test case focusing on the first fix would be really helpful. |
Usually you would be right, but this is not the case as
@andrewg-cse a simple "synthetic dem in, check output" test would suffice. No need to write C test to trigger div by zero case explicitly. Of course, if it is too much for you, I don't see a huge issue in merging this PR without tests. |
@marisn yes, re q1 != 0.0, that's exactly right, it just guards against when q1 really is exactly 0.0
|
That would be great. Artificial rasters would be a good idea. Also adding some comments in the code could help, right now some of the new variables have unclear meanings. |
I have updated to code to the current indent style using the steps from #2544 which resolves the conflict. |
…se/grass into r.sun_facing_away_from_sun_fix
I've added a unit test that covers a range of horizons and uses the North Carolina test location. |
Ok, so the tests I wrote fail when run from the CI system and I don't know why. The error from CI just says |
Not sure, when I run it locally, the test fails for me as well. Some notes though: 1.This change seems to be adding compiler warnings:
That's not the problem I guess, but it's worth fixing this. There are similar warnings coming from the existing code, but this PR at least shouldn't add new ones.
|
seems to be related: https://lists.osgeo.org/pipermail/grass-user/2023-January/083131.html |
anyone a hint how to rebase this PR to main? ;-) |
E.g: #905 (comment) |
Rebased, this is now up-to-date with main. |
some follow up test with real data of mountain area with high geomorphology variability: https://lists.osgeo.org/pipermail/grass-user/2023-March/083153.html |
real data example
r.sun cmd not patched
r.sun cmd patched
mountain summit with steep slopes in all aspect directions: aspect slope difference: |
any thoughts? |
I decided to do a test on an artificial surface (saddle shape) and the results look pretty convincing. Top row: with this PR I can write a simpler test based on this experiment, the one in this PR is problematic (complex and didn't run). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results are convincing. The tests are included. The code looks okay.
Let's merge this and include in 8.3.
This is a possible errata case (#2813).
@andrewg-cse thank you for this important contribution! |
Is this patch included in current developmental versions of GRASS 8.3.0 available here - https://wingrass.fsv.cvut.cz/grass83/ ? Or is there an easy way to apply it to my current 8.2.1 installation on Windows 11? regards, |
Yes! |
Thanks. |
I downloaded and installed https://wingrass.fsv.cvut.cz/grass83/WinGRASS-8.3.dev-c05cc14387-42-Setup.exe This comment above -
suggests that the patch will perhaps be included in version 8.4.0 ? regards, |
My elevation map consists of a small square hillock. The central zone is a flat square, with regions towards its north, south, east and west sloping downwards at 15 degrees, respectively towards north, south, east and west.
Slope and Aspect are from r.slope.aspect and lat-long are from r.latlong. Horizon rasters are from r.horizon with a step of 5 degrees. In the image, r.sun appears to show significantly greater global insolation on the north facing slope as compared to the south facing slope. For a location at latitude 34 deg N, this should not be the case. Would appreciate if someone can try to replicate my results and point out what I am doing wrong. regards, PS: My input elevation raster is: hot.txt. It is in Arc/Info ASCII Grid format, so you need to use r.in.gdal to read it into GRASS GIS - gs.run_command('r.in.gdal', flags='o', input='hot.txt', output='hotraster') |
This PR was merged in March into main aac1c5a, that is 8.3.dev, the version you tried is c05cc14 (same branch) from a couple of days ago. |
Appreciate you running my test case. I get the same results as you do if I just run r.sun.daily. But this seems to change if r.horizon is used. Can you please try the following sequence of commands - r.horizon elevation=hotraster step=05 output=horzangle distance=0.5 Or is there something wrong with this? regards, This is the output I get - |
When I compare r.sun output with and without using horizons, I get significantly different values for some areas. I don't have a good explanation, so perhaps create a new issue and show how the results of r.sun differ with and without horizons. r.sun.daily is just a wrapper around r.sun. |
Will do, Thanks. regards, |
New issue created here - regards, |
Issue was that direct beam radiation falling on the back of some steep approx north facing (in northern hemisphere) inclined surfaces is included in direct beam results. This was caused by an incorrect value of sunSlopeGeom->longit_l in the rsunlib.c, lumcline2 function. In main.c, in the calculate() function at line 1953, the value of q1 changes sign as the slope increases for north (in northern hemisphere) facing surfaces. This causes tan_lam_l to also change sign and hence the result of atan stored in sunSlopeGeom.longit_l to jump from just less than pi/2 to just more than -pi/2. The required value is pi more than this (which is another correct atan result as the tan plot repeats every pi radians). The fix detects this situation and adds pi to the result. I've done this addition in the rsunlib.c, lumcline2 function, as this is where the time offset is used. Also, there is a possibility of a divide by zero error if q1 is zero. This fix also corrects this. Co-authored-by: Anna Petrasova <kratochanna@gmail.com>
Issue was that direct beam radiation falling on the back of some steep approx north facing (in northern hemisphere) inclined surfaces was included in direct beam results. This was caused by an incorrect value of sunSlopeGeom->longit_l in the rsunlib.c, lumcline2 function.
The root cause was:
In main.c, in the calculate() function at line 1953, the value of q1 changes sign as the slope increases for north (in northern hemisphere) facing surfaces. This causes tan_lam_l to also change sign and hence the result of atan stored in sunSlopeGeom.longit_l to jump from just less than pi/2 to just more than -pi/2. The required value is pi more than this (which is another correct atan result as the tan plot repeats every pi radians). The fix detects this situation and adds pi to the result. I've done this addition in the rsunlib.c, lumcline2 function, as this is where the time offset is used.
Also, there is a possibility of a divide by zero error if q1 is zero. This change also corrects this.