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 JSON output option #2666

Merged
merged 17 commits into from
Dec 15, 2022
Merged

r.kappa: Add JSON output option #2666

merged 17 commits into from
Dec 15, 2022

Conversation

marisn
Copy link
Contributor

@marisn marisn commented Nov 24, 2022

Current output of r.kappa is not machine readable. This PR adds a new flag to print out all data in a json format for greater reusability.

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.
@marisn marisn marked this pull request as ready for review December 1, 2022 14:17
@marisn marisn requested review from nilason and wenzeslaus December 1, 2022 14:18
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 fine to me, tests succeeds and I found no regressions (tested with example in manual).

I have some minor suggestions I noted here and there, I attach patch here:
PR2666_fixes.diff.txt.

With that patch the source code for r.kappa passes -Wall -Wextra on clang without warnings, and cppcheck and scan-build are also happy.

I do miss your name in the head of main.c, and while you're there also update (c) date :-).

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/prt_json.c Outdated Show resolved Hide resolved
raster/r.kappa/prt_kappa.c Outdated Show resolved Hide resolved
raster/r.kappa/prt_kappa.c Outdated Show resolved Hide resolved
raster/r.kappa/prt_mat.c Outdated Show resolved Hide resolved
@nilason nilason added enhancement New feature or request C Related code is in C labels Dec 1, 2022
@nilason nilason added this to the 8.3.0 milestone Dec 1, 2022
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.

I'm excited to see another JSON output. I think that especially the JSON output would be better with full words rather than various abbreviations. -999 does not seem needed when we have NaN in the "common" (not standard) JSON. (C99 has INFINITY, but NAN may not be available in general, but it is a GNU extension.) Can you please sync the names with docs and interface too?

raster/r.kappa/testsuite/test_r_kappa.py Show resolved Hide resolved
raster/r.kappa/testsuite/test_r_kappa.py Outdated Show resolved Hide resolved
raster/r.kappa/testsuite/test_r_kappa.py Outdated Show resolved Hide resolved
raster/r.kappa/prt_json.c Outdated Show resolved Hide resolved
raster/r.kappa/prt_json.c Outdated Show resolved Hide resolved
raster/r.kappa/prt_json.c Outdated Show resolved Hide resolved
raster/r.kappa/testsuite/test_r_kappa.py Outdated Show resolved Hide resolved
raster/r.kappa/calc_metrics.c Outdated Show resolved Hide resolved
raster/r.kappa/main.c Outdated Show resolved Hide resolved
raster/r.kappa/prt_kappa.c Outdated Show resolved Hide resolved
@marisn marisn requested review from nilason and wenzeslaus December 3, 2022 09:01
@marisn marisn mentioned this pull request Dec 3, 2022
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.

Great work, looks good to me!

Re.: -999, it is probably the safest solution.

@wenzeslaus
Copy link
Member

Re.: -999, it is probably the safest solution.

Output - JSON: Generally, if people write in Python, chances are that JSON outputs will contain NaN without even making a decision to actually do that. A lot of reading will likely happen in Python where NaN will be accepted (read as float("nan")). Strangely, JavaScript is the one (?) place which insists on standardized JSON and one needs to use 3rd party lib to read JSON with NaNs.

Output - consistency: Using -999 for NaN would mean that also other places which output that should use the same number, but that's of course neither ensured nor feasible. ...the old NULL/no-data debate.

Code: #2681 now introduces NAN to the C code base, but I'm more concerned with outputs.

Possibly, infinity may work everywhere?

@marisn
Copy link
Contributor Author

marisn commented Dec 7, 2022

Possibly, infinity may work everywhere?

No. Then null it is. I'll change the PR & merge it.

@marisn marisn dismissed wenzeslaus’s stale review December 15, 2022 11:15

All requested changes are implemented.

@marisn marisn merged commit ffb0ce6 into OSGeo:main Dec 15, 2022
@marisn marisn deleted the r_kappa_json branch December 15, 2022 11:34
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
The old output mode is kept as a default to not break any of existing scripts.
@wenzeslaus wenzeslaus changed the title Add json output option to r.kappa r.kappaAdd JSON output option Jun 6, 2023
@wenzeslaus wenzeslaus changed the title r.kappaAdd JSON output option r.kappa: Add JSON output option Jun 6, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
The old output mode is kept as a default to not break any of existing scripts.
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
Projects
Development

Successfully merging this pull request may close these issues.

3 participants