r.fuzzy.set: correction of sshape formula in fuzzy.c

[ecodiv]

The calculation of the Sshape for when the shape parameter < 0 seems to be incorrect.

I haven't checked (yet) the other shape formulas

Note; there is another issue I am not sure how to solve. See #930

[ecodiv]

The issue reported in #930 should be solved after replacing the abs function (works with integer values) with the fabs function (works with floating and double values). Thanks to @GISjoerd for the pointer.

[wenzeslaus]

Given that shape is float, fabs is correct. I can't comment on the other change. Will you link an equation?

[ecodiv]

Given that shape is float, fabs is correct. I can't comment on the other change. Will you link an equation?

According to the manual page, the equation for the s-shape is:

sin(x * Pi/2)^m (for positive shape parameter)
1-cos(x * Pi/2)^m (for negative shape parameter)

The formula in the original code is:

return (shape < 0) ? pow(1 - cos(x * PI2), m) : pow(sin(x * PI2), m);

The question is which one is correct. I can't find the equation in one of the references provided in the manual page. Testing both shows that the formula used in the original code does not yield the expected result (shape of the curve). Using the formula from the manual does.

Using the original formula, varying the s-parameter between -1 and 1 gives the following curves (created using the R/grass code in test_fuzzy_set.zip with as input the landsat layer lsat5_1987_10@landsat from the North Carolina full dataset (https://grass.osgeo.org/download/data/).

The results shows that the results for negative shape parameters are not correct.

The equation in the PR is:

return (shape < 0) ? 1 - pow(cos(x * PI2), m) : pow(sin(x * PI2), m);

Using this equation gives the following results:

These results show (more or less) the expected s-shaped curves shown in the manual page:

[wenzeslaus]

This is great. Thank you for demonstrating why the new code is correct. I'm for merging this PR now.

I have two notes which don't prevent this PR from being merged:

1. It would be a good idea to write tests for the tool now when you consider the results to be correct. (You write the test using the current values of the tool assuming they are correct. Any change which would change these values fails the test allowing us to evaluate if the change should or should not modify the results.)
2. The bottom curves in the images look more angular, less smooth. I think I see it in both images, so maybe it is just sampling, not an issue with the code. I did not check the R code.

[ecodiv]

This is great. Thank you for demonstrating why the new code is correct. I'm for merging this PR now.

Thanks. I'll merge it.

I have two notes which don't prevent this PR from being merged:

1. It would be a good idea to write tests for the tool now when you consider the results to be correct. (You write the test using the current values of the tool assuming they are correct. Any change which would change these values fails the test allowing us to evaluate if the change should or should not modify the results.)

For now, I have some code to test locally, and perhaps it is relatively easy to convert that into an automated test, but I have no experience of doing that whatsoever. So will look into that when time allows.

2. The bottom curves in the images look more angular, less smooth. I think I see it in both images, so maybe it is just sampling, not an issue with the code. I did not check the R code.

Yes, I sampled a real RS layer, which probably has much less values in the extremes of the value range.

[wenzeslaus]

...I have some code to test locally, and perhaps it is relatively easy to convert that into an automated test, but I have no experience of doing that whatsoever. So will look into that when time allows.

The grass.gunittest testing framework allows tests in Python unittest syntax as well as shell scripts. In any case, some automated check of results is needed (as opposed to human inspecting the results). pytest-based tests are not yet checked in addons, but are actively checked in core, so I think it's okay to start adding them to addons, too.