-
-
Notifications
You must be signed in to change notification settings - Fork 313
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.report: add JSON support #3935
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Considering the following: {
"mask": "none",
"description": "no data"
} We should not output special strings for "null" values. Python uses >>> import json
>>> json.dumps({"max": None})
'{"max": null}' |
@wenzeslaus I agree that we should not output special strings for "null" values. |
Hi @wenzeslaus and @cwhite911, I have made the requested changes but the code segfaults when Thread 1 "r.report" received signal SIGSEGV, Segmentation fault.
Rast_quant_get_ith_rule (q=0x555555594808, i=-2147483648, dLow=0x7fffffffd100, dHigh=0x7fffffffd108, cLow=0x7fffffffd0d4, cHigh=0x7fffffffd0d4) at quant.c:330
330 *dLow = q->table[i].dLow;
(gdb) bt
#0 Rast_quant_get_ith_rule (q=0x555555594808, i=-2147483648, dLow=0x7fffffffd100, dHigh=0x7fffffffd108, cLow=0x7fffffffd0d4, cHigh=0x7fffffffd0d4) at quant.c:330
#1 0x00007ffff7f9041e in Rast_get_ith_d_cat (pcats=0x5555555947e0, i=-2147483648, rast1=0x7fffffffd100, rast2=0x7fffffffd108) at cats.c:1039
#2 0x000055555555a30c in make_category (ns=211, nl=0, sub_categories=0x0) at prt_json.c:75
#3 0x000055555555a68a in make_categories (start=0, end=212, level=0) at prt_json.c:110
#4 0x000055555555adfb in print_json () at prt_json.c:215
#5 0x000055555555c9bd in report () at report.c:9
#6 0x0000555555558b1e in main (argc=4, argv=0x7fffffffd598) at main.c:77 But I am not sure how to fix this issue. Can you please help with it? |
Next time, please, provide the command you used to ease reproduction of the issue. I ran the code but it did not crash. Looking at the gdb bt output, value of |
Hi @marisn! Sorry I forgot to mention the command. It was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kritibirda26 Nice job on the revision. We still have a couple of items to clean up, but it's almost there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you address the comment above I think this PR looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
The tests aren't passing, I think because it assumes a custom dataset to be present. Is there a reason the dataset (location) is different from the other tests? |
@echoix I have no idea. The test passes locally for me. I'll try to debug and fix it. |
@cwhite911 and @echoix the tests are now fixed. |
If you would've downloaded the same dataset as the one available in CI, downloaded in https://github.com/OSGeo/grass/blob/d7b13e0a63612cb98d3205eacde3acc922954533/.github/workflows/test_thorough.sh, I don't think that your last two commits were needed (maybe in part). (The name is nc_spm_full_v2alpha2) The thing is, it's not true that the grass makes the fields vary as mentioned in the test comment, it only depends on the data used. Well knowing what data was sent as a test input is kinda important to have a well defined test. After the last two commits to the test, if I understand correctly, it doesn't check if the location field is correctly filled out. (Only the presence) And with the test with extra options, is there something related to the created field that should be done (I'm not sure it is touched just by reading). I know our test infrastructure doesn't handle mocking date/time info properly to have a fully deterministic test, so the approach of "golden master" isn't possible here. Probably checking existence, and the format of it should be used. |
I'll download the CI dataset locally and update the test to check both of those fields. As for dates, yes that was an oversight. Can you please let me know if an ISO date formatter is included in grass, otherwise I'll write up the utility manually? |
I'm not the best person to answer on what the C-based code contains. Lately I'm getting better at knowing about the Python files in the repo, but even with over 391 000 lines of Python code, it's only 31% of the repo. So even if there were, I wouldn't know about it. |
Might this code help? Ah, you wanted C code: so probably |
I would expect that on Linux at least, it would be a single format code, as it's so standard. If the datetimes are only in UTC, it's even easier. Something should exist in a standard library. |
I saw that the new tests pass now! You are getting close! |
Co-authored-by: Edouard Choinière <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just running a fresh CI run, as it was 58 commits behind main, but it seems correct for now!
Co-authored-by: Nicklas Larsson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with this
* r.report: add json support * address PR feedback * fix segfault * address PR feedback * update tests to use CI dataset * change created field to iso8601 datetime format * fix created field test * fix map description field * Apply suggestions from code review Co-authored-by: Edouard Choinière <[email protected]> * update documentation * Update raster/r.report/format.c Co-authored-by: Nicklas Larsson <[email protected]> --------- Co-authored-by: Edouard Choinière <[email protected]> Co-authored-by: Nicklas Larsson <[email protected]>
* r.report: add json support * address PR feedback * fix segfault * address PR feedback * update tests to use CI dataset * change created field to iso8601 datetime format * fix created field test * fix map description field * Apply suggestions from code review Co-authored-by: Edouard Choinière <[email protected]> * update documentation * Update raster/r.report/format.c Co-authored-by: Nicklas Larsson <[email protected]> --------- Co-authored-by: Edouard Choinière <[email protected]> Co-authored-by: Nicklas Larsson <[email protected]>
Using parson, add JSON output support to r.report module.
Sample JSON output is according to the discussion in #3033, example: