-
-
Notifications
You must be signed in to change notification settings - Fork 332
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.category: Add color option #5011
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
raster/r.category/main.c
Outdated
@@ -440,3 +488,47 @@ int scan_vals(const char *s, double *x) | |||
return 1; | |||
return 0; | |||
} | |||
|
|||
void get_color(enum ColorOutput color_format, int red, int grn, int blu, |
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.
Again, I would make sure you can't reuse some of the functions from the library you wrote. Maybe it's not possible, but it would be nice to avoid code duplication.
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.
Sure, I will keep that in mind.
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.
Thank you, this looks great, just couple comments.
Signed-off-by: Nishant Bansal <[email protected]>
Co-authored-by: Anna Petrasova <[email protected]> Signed-off-by: Nishant Bansal <[email protected]>
# Replacing '\r' because Windows uses '\r\n' for line endings, but we | ||
# want to remove the '\r' (carriage return) to standardize line endings. |
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.
Did you have actual issues with this when you ran the tests on windows? That's why the replace is 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.
Yes, I tested this, and pytest was failing on Windows with an error similar to this for each test:
FAILED raster/r.category/tests/r_category_test.py::test_r_category_plain_output - AssertionError: Expected 1,trees
2,water
, but got 1,trees
2,water
assert '1,trees\r\n2,water\r\n' == '1,trees\n2,water\n'
- 1,trees
+ 1,trees
? +
- 2,water
+ 2,water
? +
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.
Perhaps we should just read it internally with text mode (aka universal newlines) to avoid the issues at this level.
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.
Perhaps we should just read it internally with text mode (aka universal newlines) to avoid the issues at this level.
See #4517?
Signed-off-by: Nishant Bansal <[email protected]>
Enhancement: Add Color Option to Category Output
Fixes: #4071
Overview
This PR introduces a new
color
option forr.category
, allowing users to retrieve category colors alongside labels. This enhancement is useful for visualization workflows where knowing both the category label and its associated color is important.Changes Made
color
option that allows specifying the output format:none
(default) – No color output.rgb
– Outputs colors inrgb(r, g, b)
format.hex
– Outputs colors in#RRGGBB
format.triplet
– Outputs colon-separatedR:G:B
values.Testing & Validation
r.category doctest
file.color
option have been added at the end.