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.colors.out: Add JSON support #4555

Merged
merged 14 commits into from
Nov 5, 2024

Conversation

NishantBansal2003
Copy link
Contributor

fixes: #3537
Use parson to add json output format support to the r.colors.out and r3.colors.out module.
The JSON output looks like as follows:

[
    {
        "value": 1,
        "rgb": "rgb(21, 5, 57)"
    },
    {
        "value": 2,
        "rgb": "rgb(143, 15, 140)"
    },
    {
        "value": 3,
        "rgb": "rgb(95, 225, 24)"
    },
    {
        "value": 4,
        "rgb": "rgb(18, 116, 26)"
    },
    {
        "value": 5,
        "rgb": "rgb(136, 220, 166)"
    },
    {
        "value": 6,
        "rgb": "rgb(250, 245, 99)"
    },
    {
        "value": 7,
        "rgb": "rgb(221, 176, 246)"
    },
    {
        "value": 8,
        "rgb": "rgb(236, 78, 1)"
    },
    {
        "value": 9,
        "rgb": "rgb(2, 197, 45)"
    },
    {
        "value": 10,
        "rgb": "rgb(17, 118, 29)"
    },
    {
        "value": 11,
        "rgb": "rgb(127, 210, 181)"
    },
    {
        "value": 12,
        "rgb": "rgb(129, 13, 6)"
    },
    {
        "value": 13,
        "rgb": "rgb(105, 239, 116)"
    },
    {
        "value": 14,
        "rgb": "rgb(242, 232, 152)"
    },
    {
        "value": 15,
        "rgb": "rgb(90, 161, 179)"
    },
    {
        "value": 16,
        "rgb": "rgb(55, 187, 209)"
    },
    {
        "value": 17,
        "rgb": "rgb(24, 9, 97)"
    },
    {
        "value": 18,
        "rgb": "rgb(127, 247, 104)"
    },
    {
        "value": 19,
        "rgb": "rgb(114, 10, 161)"
    },
    {
        "value": 20,
        "rgb": "rgb(8, 35, 6)"
    },
    {
        "value": 21,
        "rgb": "rgb(220, 204, 139)"
    },
    {
        "value": 22,
        "rgb": "rgb(160, 3, 70)"
    },
    {
        "value": 23,
        "rgb": "rgb(146, 169, 74)"
    },
    {
        "value": 24,
        "rgb": "rgb(202, 8, 65)"
    },
    {
        "value": 25,
        "rgb": "rgb(32, 45, 210)"
    },
    {
        "value": 26,
        "rgb": "rgb(33, 173, 31)"
    },
    {
        "value": 27,
        "rgb": "rgb(62, 132, 123)"
    },
    {
        "value": 28,
        "rgb": "rgb(34, 120, 179)"
    },
    {
        "value": 29,
        "rgb": "rgb(48, 38, 22)"
    },
    {
        "value": "nv",
        "rgb": "rgb(255, 255, 255)"
    },
    {
        "value": "default",
        "rgb": "rgb(255, 255, 255)"
    }
]

@github-actions github-actions bot added raster Related to raster data processing Python Related code is in Python C Related code is in C module labels Oct 20, 2024
@NishantBansal2003 NishantBansal2003 force-pushed the add-json-r-colors-out branch 2 times, most recently from dfe6501 to 8e92475 Compare October 20, 2024 12:17
raster/r.colors.out/tests/conftest.py Outdated Show resolved Hide resolved
raster/r.colors.out/tests/conftest.py Outdated Show resolved Hide resolved
raster/r.colors.out/tests/conftest.py Outdated Show resolved Hide resolved
raster/r.colors.out/tests/conftest.py Outdated Show resolved Hide resolved
raster/r.colors.out/tests/r3_colors_out_test.py Outdated Show resolved Hide resolved
@echoix
Copy link
Member

echoix commented Oct 20, 2024

The style errors are mostly about file endings. Either run pre-commit locally or use ruff and black. Applying the suggested comment might not always work, as it's missing a final newline.

@NishantBansal2003
Copy link
Contributor Author

Please re-trigger the CI...
cc: @echoix

@petrasovaa petrasovaa added this to the 8.5.0 milestone Oct 21, 2024
@github-actions github-actions bot added HTML Related code is in HTML docs labels Oct 22, 2024
@echoix
Copy link
Member

echoix commented Oct 22, 2024

Once we start having review comments, it's easier when there's no more force pushes, as it's hard to follow when the commit doesn't exist anymore.

@NishantBansal2003
Copy link
Contributor Author

Once we start having review comments, it's easier when there's no more force pushes, as it's hard to follow when the commit doesn't exist anymore.

Apologies, I initially thought the previous change was causing issues and was not relevant. I will be more careful next time.

@echoix
Copy link
Member

echoix commented Oct 22, 2024

Once we start having review comments, it's easier when there's no more force pushes, as it's hard to follow when the commit doesn't exist anymore.

Apologies, I initially thought the previous change was causing issues and was not relevant. I will be more careful next time.

There's no review comments (from us) yet, so you're doing fine ;)

@echoix
Copy link
Member

echoix commented Oct 22, 2024

But take a look at the build failures already here. Are you able to debug them locally, by doing a build on your side and fixing them ?

@NishantBansal2003
Copy link
Contributor Author

I'm not sure what is causing the error in the macOS and GNU build CI with gnu17 and c++17. I tried running everything locally on macOS, and it works fine. Could you assist me in troubleshooting this issue?Screenshot 2024-10-22 at 1 36 13 PM

@echoix
Copy link
Member

echoix commented Oct 22, 2024

I've found one error line on the macOS one: 8905-8907:

https://github.com/OSGeo/grass/actions/runs/11455850645/job/31882348189#step:10:8906


prt_json.c:11:28: error: unused parameter 'fp' [-Werror,-Wunused-parameter]
   11 |                      FILE *fp, JSON_Array *root_array, int perc)
      |                            ^
1 error generated.

image

So maybe try locally with the same flags as the GNU standards check job, to ensure the unused parameters are triggered as errors so you see them

@echoix
Copy link
Member

echoix commented Oct 22, 2024

Also make sure that you have correctly cleaned locally to make sure you build everything to see the potential issues

@NishantBansal2003
Copy link
Contributor Author

Also make sure that you have correctly cleaned locally to make sure you build everything to see the potential issues

Sure, thanks! Fixing the issues now...

raster/r.colors.out/prt_json.c Show resolved Hide resolved
raster/r.colors.out/raster_main.c Outdated Show resolved Hide resolved
@wenzeslaus
Copy link
Member

@cwhite911 Can you give your opinion on representing the special values - default and nulls (nv) and on representing the color itself? rgb(...) looks good, but GRASS GIS currently reads #RRGGBB and RRR:GGG:BBB. We can add support for rgb(...), output more than one format, ...

@wenzeslaus
Copy link
Member

...define the individual color output as "intersection between HTML/CSS formats and GRASS GIS supported formats" so now hex format, but possibly rgb(...) in the future. You would be be at loss parsing it manually, but it would work just fine for passing it to HTML-aware function or back to GRASS GIS.

raster/r.colors.out/local_proto.h Outdated Show resolved Hide resolved
@neteler
Copy link
Member

neteler commented Oct 22, 2024

run pre-commit locally

(for this, see https://github.com/OSGeo/grass/blob/main/doc/development/style_guide.md#using-pre-commit)

@NishantBansal2003
Copy link
Contributor Author

Is there anything that needs to be addressed?
cc: @cwhite911

Copy link
Contributor

@cwhite911 cwhite911 left a comment

Choose a reason for hiding this comment

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

Hi @NishantBansal2003. Nice job working on this PR! I've added a few comments for some improvements and had a few questions as well.

Will you add HEX or any other color formats as outputs?

raster/r.colors.out/prt_json.c Show resolved Hide resolved
raster/r.colors.out/prt_json.c Show resolved Hide resolved
raster/r.colors.out/prt_json.c Show resolved Hide resolved
raster/r.colors.out/r.colors.out.html Outdated Show resolved Hide resolved
raster/r.colors.out/r3.colors.out.html Show resolved Hide resolved
raster/r.colors.out/tests/conftest.py Show resolved Hide resolved
Signed-off-by: Nishant Bansal <[email protected]>
@NishantBansal2003
Copy link
Contributor Author

Rebasing because I was getting some unrelated errors in the last CI run.

@NishantBansal2003
Copy link
Contributor Author

Rebasing because I was getting some unrelated errors in the last CI run.

Hey @echoix could you retrigger the CI, need to check if the errors of last CI run was resolved or not, since it is working fine in my local.

@NishantBansal2003
Copy link
Contributor Author

Rebasing because I was getting some unrelated errors in the last CI run.

Hey @echoix could you retrigger the CI, need to check if the errors of last CI run was resolved or not, since it is working fine in my local.

Looks like there's an issue in my code. Can you help me debug it?

@echoix
Copy link
Member

echoix commented Nov 3, 2024

I don't understand yet... What is bothering me is tests on linux, without Pytest, on seemingly unrelated modules are failing too.. Is there something on the C library side that collides or something? a header problem?

@echoix
Copy link
Member

echoix commented Nov 3, 2024

I think something changed for macOS, a new run on merging the repo's main branch to my fork ends up failing while it wasn't failing on the upstream repo. Our env cache clears once a week, probably on sundays.

@NishantBansal2003
Copy link
Contributor Author

NishantBansal2003 commented Nov 4, 2024

I think something changed for macOS, a new run on merging the repo's main branch to my fork ends up failing while it wasn't failing on the upstream repo. Our env cache clears once a week, probably on sundays.

Some pytest tests are also failing in unrelated modules (https://github.com/OSGeo/grass/actions/runs/11643760411/job/32442951891?pr=4555#step:12:7654), and the same pytest tests are also failing locally for me. I'm not sure why this issue is occurring; is there something I'm missing?

EDIT: Never mind, I fixed the pytest failure(in local), but I'm not sure why the pytest fails without the last commit.

Signed-off-by: Nishant Bansal <[email protected]>
@wenzeslaus
Copy link
Member

...would it be possible for reviewers to discuss and agree on changes before submitting their requests? 🤔 This could save time and ensure that we’re all aligned on the final direction of the PR.

I'm afraid the platform to discuss it is here, and you are part of the discussion. I know it is hard to determine the tone of the comments in the discussion, so many thanks for your patience with this PR.

With 104 conversation comments and the code being in a good shape from the beginning, I would prefer to merge this soon. I will check once more and aim at approving review, but I will be happy if someone else is faster in doing that. We may want to wait for macOS CI to be fixed before actually merging this.

@petrasovaa
Copy link
Contributor

I suggest to merge this as is because any further discussion here is getting confusing. If devs have some tweaks, I suggest to open a new PR. Thanks @NishantBansal2003 for your patience!

@NishantBansal2003
Copy link
Contributor Author

Thank you, reviewers. I’ve gained valuable insights through this PR, and your feedback has been instrumental in my learning process. I apologise if my tone seemed off in any of our discussions or if I was overly opinionated or asked too many questions at times. I’ll work on improving in this regard and am committed to contributing further to the GRASS GIS community 🌍💚.

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.

Thank you @NishantBansal2003! And thanks everyone for their opinionated opinions! :-)

@wenzeslaus wenzeslaus changed the title r.colors.out: added json support r.colors.out: Add JSON support Nov 5, 2024
@echoix echoix merged commit 760d763 into OSGeo:main Nov 5, 2024
27 checks passed
@a0x8o a0x8o mentioned this pull request Nov 6, 2024
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Nov 11, 2024
* r.colors.out: added json output

Signed-off-by: Nishant Bansal <[email protected]>

* fixed CI build issues

Signed-off-by: Nishant Bansal <[email protected]>

* refactor code

Signed-off-by: Nishant Bansal <[email protected]>

* added more color formats and tests

Signed-off-by: Nishant Bansal <[email protected]>

* additional changes based on review

Signed-off-by: Nishant Bansal <[email protected]>

* fixes prototype declaration

Signed-off-by: Nishant Bansal <[email protected]>

* fixes test

Signed-off-by: Nishant Bansal <[email protected]>

* added option instead of flags

Signed-off-by: Nishant Bansal <[email protected]>

* Add a standard parser option for color formatting

Signed-off-by: Nishant Bansal <[email protected]>

* added changes based on review

Signed-off-by: Nishant Bansal <[email protected]>

* fixes function name

Signed-off-by: Nishant Bansal <[email protected]>

* fixes pytest failure

Signed-off-by: Nishant Bansal <[email protected]>

* Update lib/gis/parser_standard_options.c

---------

Signed-off-by: Nishant Bansal <[email protected]>
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 docs general HTML Related code is in HTML libraries module Python Related code is in Python raster Related to raster data processing
Projects
Development

Successfully merging this pull request may close these issues.

[Feat] Add JSON output to r.colors.out
7 participants