i.smap: fix possible pole error with log in extract function

[ymdatta]

Using logarithm function call with zero argument will lead to a pole error, which occurs if the mathematical function has an exact infinite result.

Refactor the conditional to only execute the code when number of subclasses are more than 1, which would preemptively stop us from getting into situation where log would have a zero argument. In cases where number of subclasses are <= 0, set the default value to 0. This is necessary as ll points to malloc'd memory and can contain renadom value.

References:

[1] https://en.cppreference.com/w/c/numeric/math/log [2] SEI CERT C Coding Standard

[nilason]

I gather this issue originates from cppcheck, this is the relevant part for this:

cppcheck imagery/i.smap/model.c
...
imagery/i.smap/model.c:158:43: warning: Invalid log() argument nr 1. The value is 0 but the valid values are '4.94066e-324:'. [invalidFunctionArg]
                        ll[i][j][m] = log(subsum) + maxlike;
                                          ^
imagery/i.smap/model.c:154:34: note: Assignment 'subsum=0', assigned value is 0
                        subsum = 0;
                                 ^
imagery/i.smap/model.c:155:39: note: Assuming condition is false
                        for (k = 0; k < C->nsubclasses; k++)
                                      ^
imagery/i.smap/model.c:158:43: note: Invalid argument
                        ll[i][j][m] = log(subsum) + maxlike;

The static analysis, takes the path where C->nsubclasses are 0, which leaves subsum with the initialised value of 0.0. log(0) return "-infinity and raise the "divide-by-zero" floating-point exception"[^1], which is the real issue here.

Now, in this branch of the if-else statement:

https://github.com/OSGeo/grass/blob/bc9cb2a3f9540b031a6b8e8e7d7257ca01492d3e/imagery/i.smap/model.c#L144-L160

there are two loops over C->nsubclasses which only does anything if C->nsubclasses > 1.

Wouldn't it better to change the else to a else if (C->nsubclasses > 1) on line 144, which guarantees that the for-loops are entered and the initialised value of subsum is altered?

[^1]: man 3 log

[ymdatta]

@nilason : I agree with your suggestion, it's a much cleaner way. Thanks!

I have modified the code to reflect that and I also added an else condition, which should cover the cases where nsubclasses <= 0 to set the value for ll to 0.0.

[echoix]

@jayneel-shah18 @marisn Could this be one of the problems observed in i.smap?

[echoix]

Updated since there are some tests now for i.smap.

[echoix]

I tried this branch merged into a CI setup that was consistently hitting the failed i.smap test with mismatch values (key, reference, actual): [('max', 4.495414, 3.60080480575562)].

Usually, it is difficult to reproduce, but on ubuntu 22.04 in github actions CI, with llvm (llvm19), and source-based code coverage enabled, it is practically always hit.

The issue described might be related to #2970, and #5552 tried to address it.

I tried out this branch to see if it helped, but no, the problem is consistently hit the same way.