r.kappa: Fix failures, garbage output, fallback to category values #2573
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
Although original code lacks any explanation why NA should not be printed for the first raster category, I do suspect it stems from idea that the first cat is 0 and before proper NULL support 0 was "no data" value.
![github-advanced-security[bot]](/~https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
wenzeslaus
reviewed
Sep 14, 2022
There are more issues as some values are calculated incorrectly if input data is not sequential numbers starting at 0. Thus just a draft as more work is needed.
I was thinking of shell script style with "-g" flag, but going for JSON also is an option. In separate PR, of course, after this PR is merged as this PR will have to be backported. |
These tests should fail as C code fixes will be in upcoming commits.
pii, pi, pj and pii all are arrays with size of ncat thus when nstats < ncat it was causing out of bounds reads and writes. A resulting segfault was reported in: https://trac.osgeo.org/grass/ticket/3978 A test covering this error already was added in previous commit.
wenzeslaus
approved these changes
Oct 5, 2022
There was a problem hiding this comment.
The code changes make sense to me in light of the individual commit messages (please include these descriptions into the final commit message, you could add pieces of them as comments too).
I executed the tests locally with the original code and in the one case where it actually runs and gives results, I get the same values as the ones in the tests, so the outputs which are working were not broken.
Thank you for the extra effort with the correctness test! I was really just thinking some "current values" check to see the before and after difference (or no difference).
I only noticed sub-optimal use of split in Python in tests which would be nice to have before merge.
wenzeslaus
changed the title
neteler
added a commit
to neteler/grass
that referenced
this pull request
Nov 10, 2022
Selected r.kappa indentation update from OSGeo#2544 (needed for backport r.kappa fixes in OSGeo#2573)
neteler
added a commit
that referenced
this pull request
Nov 10, 2022
neteler
pushed a commit
that referenced
this pull request
Nov 10, 2022
…2573) * use raster category values in matrix output if labels are not present * print NAs also for the first category Although original code lacks any explanation why NA should not be printed for the first raster category, I do suspect it stems from idea that the first cat is 0 and before proper NULL support 0 was "no data" value. * r.kappa: fix incorrect memory access and improve edge case handling pii, pi, pj and pii all are arrays with size of ncat thus when nstats < ncat it was causing out of bounds reads and writes. A resulting segfault was reported in: https://trac.osgeo.org/grass/ticket/3978 * tests for most of functionality
ninsbl
pushed a commit
to ninsbl/grass
that referenced
this pull request
Feb 17, 2023
…SGeo#2573) * use raster category values in matrix output if labels are not present * print NAs also for the first category Although original code lacks any explanation why NA should not be printed for the first raster category, I do suspect it stems from idea that the first cat is 0 and before proper NULL support 0 was "no data" value. * r.kappa: fix incorrect memory access and improve edge case handling pii, pi, pj and pii all are arrays with size of ncat thus when nstats < ncat it was causing out of bounds reads and writes. A resulting segfault was reported in: https://trac.osgeo.org/grass/ticket/3978 * tests for most of functionality
marisn
added a commit
to marisn/grass
that referenced
this pull request
Jun 2, 2023
…SGeo#2573) * use raster category values in matrix output if labels are not present * print NAs also for the first category Although original code lacks any explanation why NA should not be printed for the first raster category, I do suspect it stems from idea that the first cat is 0 and before proper NULL support 0 was "no data" value. * r.kappa: fix incorrect memory access and improve edge case handling pii, pi, pj and pii all are arrays with size of ncat thus when nstats < ncat it was causing out of bounds reads and writes. A resulting segfault was reported in: https://trac.osgeo.org/grass/ticket/3978 * tests for most of functionality
neteler
pushed a commit
to nilason/grass
that referenced
this pull request
Nov 7, 2023
…SGeo#2573) * use raster category values in matrix output if labels are not present * print NAs also for the first category Although original code lacks any explanation why NA should not be printed for the first raster category, I do suspect it stems from idea that the first cat is 0 and before proper NULL support 0 was "no data" value. * r.kappa: fix incorrect memory access and improve edge case handling pii, pi, pj and pii all are arrays with size of ncat thus when nstats < ncat it was causing out of bounds reads and writes. A resulting segfault was reported in: https://trac.osgeo.org/grass/ticket/3978 * tests for most of functionality
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.
r.kappa has some strange design decisions and produces strange or outright wrong output if input data doesn't match its expectations.
This PR fixes: