i.cca: use 0 based array indexing #3239
Conversation
When ccmath library was introduced in 49b8066 most of arrays were moved to 0 based indexing but some locations got missed. Most likely it is the source of a crash reported a few years a go: https://trac.osgeo.org/grass/ticket/2297
|
Although this PR fixes segfault, the code does not produce correct output (in comparison to pre- 49b8066 version). Unless the testcase passes, this PR does not improve on the current state. |
imagery/i.cca/main.c
Outdated
| cov[i][j][k] = cov[i][k][j] = sigs.sig[i - 1].var[j - 1][k - 1]; | ||
| for (j = 0; j < bands; j++) { | ||
| mu[i][j] = sigs.sig[i].mean[j]; | ||
| for (k = 0; k < j; k++) { |
There was a problem hiding this comment.
try
| for (k = 0; k < j; k++) { | |
| for (k = 0; k <= j; k++) { |
because j is not the upper bound of k to be excluded but to be included, also with zero-based indexing.
There was a problem hiding this comment.
Thanks for spotting my mistake!
Still even with this being fixed, output is not identical to the old version. Should it be identical? Suggestions how to test?
There was a problem hiding this comment.
The functions in stats.c assume zero-based indices, thus I would expect different results in the sense that results with your PR (zero based indices) are correct whereas previous results (one based indices) were not correct, particularly because the indexing to cov[][][] did not change.
You would need some independent, trusted alternative for Canonical components analysis in order to decide which results are correct. Regarding the code, your PR seems like a bugfix, previous results are probably not correct.
There was a problem hiding this comment.
Just for clarification, cov has been indexed previously in main.c as cov[i][j][k] which was wrong for one-based i, j, k because zero-based i, j, k are used in stats.c. With your PR, cov is consistently indexed with zero-based i, j, k, that could explain the differences.
github-actions
bot
added
Python
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| License (>=v2). Read the file COPYING that comes with GRASS | ||
| for details. | ||
| """ | ||
| from grass.script.core import tempname |
There was a problem hiding this comment.
[black] reported by reviewdog 🐶
| from grass.script.core import tempname | |
| from grass.script.core import tempname |
When ccmath library was introduced in 49b8066 most of arrays were moved to 0 based indexing but some locations got missed. Most likely it is the source of a crash reported a few years a go: https://trac.osgeo.org/grass/ticket/2297
With this fix I'm able to run the code without a segfault, but I haven't figured out how to get any output and thus no tests are provided.