i.gensigset: fix possible pole and divide by zero errors in regroup #4500
+11
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ymdatta
commented
Oct 11, 2024
imagery/i.gensigset/subcluster.c
Outdated
| if (subsum > 0) | ||
| likelihood += log(subsum) + maxlike; | ||
|
|
||
| if (subsum) { |
There was a problem hiding this comment.
Didn't combine both of these in the same conditional as subsum being negative is not valid for log, but valid while calculating Data.
Using logarithm function call with zero argument will lead to a pole error, which occurs if the mathematical function has an exact infinite result. Check if the argument value is zero before passing that to the log function to avoid such errors. I also added check for negative numbers just to make sure the argument is in the right domain for the log function as well. There was also a possible divide by zero scenario when we were dividing the class data by subsum, which can be zero. Added a conditional check which avoids going to that stage. Signed-off-by: Mohan Yelugoti <[email protected]>
ymdatta
force-pushed
the
cppcheck_imagery_log2
branch
from
3f08d03 to
2b1b9e6
Compare
ymdatta
added a commit
to ymdatta/grass
that referenced
this pull request
Nov 27, 2024
Documented each supression issue with comments to distinguish between false positives and true positives awaiting resolution. For the false positives supressions, appropriate information is provided on why those were considered as false positive. True positives will be removed from the suppression file once their corresponding fixes(OSGeo#4702, OSGeo#4638, OSGeo#4500, OSGeo#4499) are merged. Run: `cppcheck --suppressions-list=.cppcheck-supressions <path>` Signed-off-by: Mohan Yelugoti <[email protected]>
ymdatta
added a commit
to ymdatta/grass
that referenced
this pull request
Nov 27, 2024
Documented each suppression issue with comments to distinguish between false positives and true positives awaiting resolution. For the false positives suppressions, appropriate information is provided on why those were considered as false positive. True positives will be removed from the suppression file once their corresponding fixes(OSGeo#4702, OSGeo#4638, OSGeo#4500, OSGeo#4499) are merged. Run: `cppcheck --suppressions-list=.cppcheck-suppressions <path>` Signed-off-by: Mohan Yelugoti <[email protected]>
|
Maybe a fresh, updated CI run might be wanted before a merge, last run is getting old |
|
Can someone verify if returning 0 in these cases makes sense domain-specific-wize? For example, does it break usages to have 0 when unexpected without indications? Could we still rely on the results? Programming-wize, and math-wize, this looks fine |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using logarithm function call with zero argument will lead to a pole error, which occurs if the mathematical function has an exact infinite result.
Check if the argument value is zero before passing that to the log function to avoid such errors. I also added check for negative numbers just to make sure the argument is in the right domain for the log function as well.
There was also a possible divide by zero scenario when we were dividing the class data by subsum, which can be zero. Added a conditional check which avoids going to that stage.