Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r.kappa: Add MCC #2680

Merged
merged 21 commits into from
Feb 14, 2023
Merged

r.kappa: Add MCC #2680

merged 21 commits into from
Feb 14, 2023

Conversation

marisn
Copy link
Contributor

@marisn marisn commented Dec 3, 2022

This PR adds Matthews (Mattheus) Correlation Coefficient as one of indices provided by r.kappa.

Should be applied on the top of PR #2666

@marisn marisn requested review from wenzeslaus and nilason December 3, 2022 13:16
@marisn marisn added raster Related to raster data processing C Related code is in C labels Dec 3, 2022
@marisn marisn added this to the 8.3.0 milestone Dec 3, 2022
@marisn
Copy link
Contributor Author

marisn commented Dec 15, 2022

Ping @wenzeslaus @nilason

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Neumaier's Kahan-Babuska algorithm fits with the version in Wikipedia. I did not check MCC itself.

Given that Kahan-Babuska is applied to existing code too, this may change some results, no? I see it does not change any of the tested code. I think it is the right thing to do and that we can do that between minor versions, but we may need to emphasize that in the release notes (as a separate item in "changed" besides addition to the MCC item).

I'm asking for some code tweaks in the comments.

raster/r.kappa/calc_metrics.c Outdated Show resolved Hide resolved
raster/r.kappa/testsuite/test_r_kappa.py Show resolved Hide resolved
raster/r.kappa/calc_metrics.c Outdated Show resolved Hide resolved
raster/r.kappa/calc_metrics.c Outdated Show resolved Hide resolved
raster/r.kappa/calc_metrics.c Outdated Show resolved Hide resolved
@marisn
Copy link
Contributor Author

marisn commented Dec 23, 2022

Given that Kahan-Babuska is applied to existing code too, this may change some results, no? I see it does not change any of the tested code. I think it is the right thing to do and that we can do that between minor versions, but we may need to emphasize that in the release notes (as a separate item in "changed" besides addition to the MCC item).

In most of cases results shouldn't change. NKB will give different outcome only in rare corner cases or when n is large.

@marisn marisn requested a review from wenzeslaus January 15, 2023 18:15
@wenzeslaus wenzeslaus added the enhancement New feature or request label Feb 10, 2023
@wenzeslaus
Copy link
Member

Is there a some kind of style guideline [for using a += b instead of a = a + b]? I do use += for counters and accumulators in loops but prefer to have all terms explicit when summation is neither of them.

While in math it is common to have the terms explicit, the current programming practices usually aim primarily at documenting intention given that people spend most of the time by reading code (theirs or someone else's), so a += b comes out better because it clearly says add b to a.

In the following code:

    p0 = p0 + p0c;
    pC = p0 + pCc;

did I really mean to do p0 + pCc or is it a copy-paste error and I actually mean pC + pCc?

When using +=, it becomes clear:

    p0 += p0c;
    pC += pCc;

Clearly I meant to add to the existing value. If I would really mean p0 + pCc, I would write:

    p0 += p0c;
    pC = p0 + pCc;

@marisn marisn merged commit 7aab4cc into OSGeo:main Feb 14, 2023
@marisn
Copy link
Contributor Author

marisn commented Feb 14, 2023

did I really mean to do p0 + pCc or is it a copy-paste error and I actually mean pC + pCc?

This is a good reason. Thank you for pointing out to this aspect of coding style.

@wenzeslaus
Copy link
Member

You are welcome. I'm sorry I was not more clear before.

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* Report Matthews (Mattheus) Correlation Coefficient as one of measures
* Use the Neumaier's Kahan-Babuska algorithm to minimize errors during summation
@wenzeslaus wenzeslaus changed the title Add MCC to r.kappa r.kappa: Add MCC Jun 6, 2023
@marisn marisn deleted the r_kappa_mcc branch October 22, 2023 09:57
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* Report Matthews (Mattheus) Correlation Coefficient as one of measures
* Use the Neumaier's Kahan-Babuska algorithm to minimize errors during summation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C enhancement New feature or request raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants