d.legend: handle 0 for log (-l) #1507

HuidaeCho · 2021-04-06T23:50:36Z

Attempt to address #1482

…rasters

HuidaeCho · 2021-04-07T00:32:50Z

TODO: This module needs indent after this PR is merged.

HuidaeCho · 2021-04-07T00:35:58Z

@neteler Please test it with negative, non-positive, negative to positive, and 0 to positive rasters. It shouldn't affect positive rasters, but just in case test them too. It still doesn't support absolute logarithm.

Noticed that the flag name (-l) is different from -g from r.colors, but -l makes more sense and is reserved for r.colors.

neteler · 2021-04-11T06:49:04Z

Thanks a lot!
For testing, I applied the PR to G78 and got this compilation warning (just FYI):

grass78_git/display/d.legend]$ make
gcc  -O3 -march=native -fdiagnostics-color   -I/home/mundialis/software/grass78_git/dist.x86_64-pc-linux-gnu/include -I/home/mundialis/software/grass78_git/dist.x86_64-pc-linux-gnu/include    -DPACKAGE=\""grassmods"\"   -I/home/mundialis/software/grass78_git/dist.x86_64-pc-linux-gnu/include -I/home/mundialis/software/grass78_git/dist.x86_64-pc-linux-gnu/include -DRELDIR=\"display/d.legend\" -o OBJ.x86_64-pc-linux-gnu/draw.o -c draw.c
draw.c: In function ‘draw’:
draw.c:273:40: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 4 [-Wformat-overflow=]
  273 |                 sprintf(DispFormat, "%%%dd", (int)(log10(fabs(maxCat))) + 1);
      |                                        ^~
draw.c:273:37: note: directive argument in the range [-2147483647, 2147483647]
  273 |                 sprintf(DispFormat, "%%%dd", (int)(log10(fabs(maxCat))) + 1);
      |                                     ^~~~~~~
In file included from /usr/include/stdio.h:866,
                 from /home/mundialis/software/grass78_git/dist.x86_64-pc-linux-gnu/include/grass/gis.h:24,
                 from draw.c:14:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:38:10: note: ‘__builtin___sprintf_chk’ output between 4 and 14 bytes into a destination of size 5
   38 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   39 |       __bos (__s), __fmt, __va_arg_pack ());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Here my user test:

not more min=0 related error, great
the maximum shown is not that reported thru stats (see below, but I didn't manage to reproduce it!):

GRASS 7.8.6dev (latlong_wgs84):~ > r.info -r worldpop_2020_1km_aggregated_UNadj
min=0
max=52820.77
GRASS 7.8.6dev (latlong_wgs84):~ > r.univar worldpop_2020_1km_aggregated_UNadj -g | grep min
min=0

d.legend -s -d -l raster=worldpop_2020_1km_aggregated_UNadj@worldpop_south_america title=worldpop_2020_1km_aggregated_UNadj font=Vera

HuidaeCho · 2021-04-16T03:54:10Z

* the maximum shown is not that reported thru stats (see below, but I didn't manage to reproduce it!):

@neteler I couldn't reproduce it. Can you share your data (worldpop_2000_1km_aggregated_UNadj)?

HuidaeCho · 2021-04-16T05:06:21Z

draw.c:273:40: warning: ‘%d’ directive writing between 1 and 11 bytes into a region of size 4 [-Wformat-overflow=]
273 | sprintf(DispFormat, "%%%dd", (int)(log10(fabs(maxCat))) + 1);
| ^~
draw.c:273:37: note: directive argument in the range [-2147483647, 2147483647]
273 | sprintf(DispFormat, "%%%dd", (int)(log10(fabs(maxCat))) + 1);
| ^~~~~~~

I think you use GCC >= 7 (https://gcc.gnu.org/gcc-7/changes.html). I'm stuck with GCC 5 :-(. The warning is because DispFormat is not big enough. Buffer overflow can actually happen if maxCat is very large such that log10(maxCat)+1 >= 100. That is, maxCat >= 10^99, which looks pretty safe... One solution would be DispFormat[7] (two more bytes) and

sprintf(DispFormat, "%%%dd", (char)(log10(fabs(maxCat))) + 1);

char ranges from -128 to 127 so it can take up four bytes max plus %, d, and \0.

Now, if maxCat is very small and log10(fabs(maxCat)) becomes < -1, text will be left-aligned unintentionally.

In general, I think this module needs a major refactoring including the above warnings, uninitialized variables, indentation, breaking long code, etc.

neteler · 2021-04-19T18:42:57Z

* the maximum shown is not that reported thru stats (see below, but I didn't manage to reproduce it!):
@neteler I couldn't reproduce it. Can you share your data (worldpop_2000_1km_aggregated_UNadj)?

(btw: gcc version 10.2.1 here)

My use case is this one:

#1482 (comment)

but I'll test again to see if it happens again + (try to) provide synthetic test data generated with r.random.* etc.

neteler · 2021-04-19T21:15:02Z

@neteler Please test it with negative, non-positive, negative to positive, and 0 to positive rasters. It shouldn't affect positive rasters, but just in case test them too. It still doesn't support absolute logarithm.

OK, I have set up some test cases (see below). Hope they are somehow meaningful...

# NC sample dataset
g.region -p n=228500 s=215000 w=630000 e=645000 res=100 -p
d.mon wx0

Negative values

# negative, DCELL
r.surf.random out=random_negative min=-100 max=-1
r.univar random_negative
r.info -r random_negative
min=-99.9995498657227
max=-1.01180768013

d.rast random_negative
d.legend raster=random_negative@user1 title=random_negative
d.legend -l raster=random_negative@user1 title="Log random_negative"

Negative to positive

# negative to positive, DCELL
r.surf.random out=random_negative_positive min=-100 max=100
r.univar random_negative_positive
r.info -r random_negative_positive
min=-99.9983367919922
max=99.9910354614258

d.rast random_negative_positive
d.legend raster=random_negative_positive@user1 title=random_negative_positive
d.legend -l raster=random_negative_positive@user1 title="Log random_negative_positive"

0 to positive

# 0 to positive rasters, DCELL
r.surf.random out=tmp min=0 max=100
r.mapcalc "random_positive = if( tmp < 1.0, 0.0, tmp)"
g.remove raster name=tmp -f
r.univar random_positive
r.info -r random_positive
min=0
max=99.9885711669922

d.rast random_positive
d.legend raster=random_positive@user1 title=random_positive
d.legend -l raster=random_positive@user1 title="Log random_positive"

veroandreo · 2021-04-22T17:39:05Z

These are my tests with and without this PR:

Without the PR:

positive values starting from 0.something (lots of small values some pretty high)
negative and positive values - only legend without -l can be drawn, for -l, I get the reported error

Failed to run command 'd.legend -b -l raster=prueba_grib@pruebas title="With -l flag" at=5,50,47,50'. Details:
GRASS_INFO_ERROR(207604,1): Range [-69.550, 42.850] out of the logarithm domain.
GRASS_INFO_END(207604,1)

With this PR:

looks good - same as before the PR
legend with -l does not look good, only positive values shown, though I no longer get the error

I wonder if it really make sense to try to plot a logarithmic scale with negative data... For the case of value zero, can't we just add 1 to get 0 as a result?

Also, while no error is reported for negative data, I believe it's worthy to raise a warning at least

veroandreo · 2021-04-22T17:43:35Z

@neteler Please test it with negative, non-positive, negative to positive, and 0 to positive rasters. It shouldn't affect positive rasters, but just in case test them too. It still doesn't support absolute logarithm.

OK, I have set up some test cases (see below). Hope they are somehow meaningful...

I find the legends pretty odd, tbh, even the one for all positive numbers... I would expect colors to be more equally distributed as in my case 1 examples. Maybe the distribution of values plays a role?

HuidaeCho · 2021-04-23T22:59:56Z

Let me summarize what this PR does:

For a positive raster, everything is good. Output should be exactly the same as without this PR.
For a 0-to-positive or negative-to-positive raster, use 1 as the min color
- It can be a problem when the raster's max is smaller than 1. This is why it would be great to know the true positive min value.
- We also skip all raster colors between the min and 1.
For a negative raster, use the max color only.
- Actually, this behavior can be wrong most of the case unless the raster is colorized using r.colors -g, which does use the max color only (Rast_log_colors: Use a sane default color for non-positive rasters and assume a positive min cell value #1481), but there is no smart way to know if this is the case.
- Probably, we should throw a fatal error in this case because the legend won't match the display most of the case. Commented as XXX in the PR.

HuidaeCho · 2021-04-23T23:03:58Z

@neteler Please test it with negative, non-positive, negative to positive, and 0 to positive rasters. It shouldn't affect positive rasters, but just in case test them too. It still doesn't support absolute logarithm.

OK, I have set up some test cases (see below). Hope they are somehow meaningful...

I find the legends pretty odd, tbh, even the one for all positive numbers... I would expect colors to be more equally distributed as in my case 1 examples. Maybe the distribution of values plays a role?

I think your example 1 is a good case for a logarithmic legend. Data looks logarithmically distributed than linearly. The log legend looks good to me.

HuidaeCho · 2021-04-23T23:19:15Z

Now, fatal error on a non-positive raster and warnings on min <= 0.

neteler · 2023-02-18T10:23:54Z

This PR looks good, i.e., it improves the current situation. Merge it?

petrasovaa

I think this implementation is improvement, but the PR needs to be cleaned up, see my other comments.

petrasovaa · 2023-02-20T16:02:09Z

display/d.legend/draw.c

+                            "discarding colors for cells <= %g and "
+                            "assuming a positive min cell value of %g"),
+                          eps, eps);
+                eps_min = eps;


I wonder if you couldn't just use dmin = eps_min here, which would avoid all the changes below.

petrasovaa · 2023-02-20T16:07:38Z

display/d.legend/draw.c

+                    /* XXX: for log scale, if the entire raster is not
+                     * positive, use the max color only; actually, this is
+                     * incorrect because negative values can have different
+                     * colors on the monitor; maybe, we should throw a fatal
+                     * error in this case because using the max color only for
+                     * displaying a raster and its legend is different and the
+                     * legend must always match the display; well... unless the
+                     * raster is colorized using r.colors -g, but how do we
+                     * know that? */
+                    val = dmax <= 0 ? dmax : eps_min * pow(dmax / eps_min, num);


All these comments are somewhat confusing and perhaps not updated after introducing the fatal error for rasters with all negative values. I would suggest to summarize the reasoning and perhaps move it somewhere close to the fatal error call.

Given the fatal error in the beginning, these expressions don't need to test for the negative cases and can be simplified. This applies to all/most of the changes below.

HuidaeCho added 4 commits April 6, 2021 19:49

d.legend: handle 0 for log (-l)

b5e8c1d

Minor revision

0642c6b

Comments

15374b3

log support for label_values; fix min and max labels for non-positve …

49db556

…rasters

HuidaeCho requested a review from neteler April 7, 2021 00:37

HuidaeCho added enhancement New feature or request raster Related to raster data processing labels Apr 7, 2021

Add comments

10b4e31

Fatal error on max <= 0 (non-positive raster)

d70a3a6

HuidaeCho mentioned this pull request Apr 23, 2021

Rast_log_colors: Use a sane default color for non-positive rasters and assume a positive min cell value #1481

Closed

HuidaeCho mentioned this pull request Oct 4, 2021

r.colors fix -g and -a (#1480) #1923

Closed

HuidaeCho requested a review from susi October 4, 2021 12:59

neteler mentioned this pull request Oct 10, 2021

[Bug] CI error: flake8 Expected - Waiting for status to be reported #1935

Closed

neteler added this to the 8.0.1 milestone Dec 9, 2021

ninsbl modified the milestones: 8.0.1, 8.0.2 Feb 20, 2022

wenzeslaus modified the milestones: 8.0.2, 8.4.0 Feb 27, 2022

wenzeslaus and others added 4 commits September 7, 2022 04:43

Indent code

0e337b6

Merge main with indent into d_legend_log with indent on draw.c

77bcdd1

apply clang-format

e8f0ab7

Merge branch 'main' into d_legend_log

5cbbb30

nilason added the C Related code is in C label Feb 15, 2023

Merge branch 'main' into d_legend_log

bb285a6

petrasovaa requested changes Feb 20, 2023

View reviewed changes

wenzeslaus modified the milestones: 8.3.0, 8.4.0 Mar 17, 2023

Merge remote-tracking branch 'upstream/main' into d_legend_log

79b2221

HuidaeCho self-assigned this Mar 6, 2024

Merge branch 'main' into d_legend_log

1e26114

github-actions bot added module display labels Mar 21, 2024

wenzeslaus modified the milestones: 8.4.0, Future Apr 27, 2024

echoix added 2 commits June 25, 2024 23:36

Merge branch 'main' into d_legend_log

6c271f7

Merge branch 'main' into d_legend_log

74f0487

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

d.legend: handle 0 for log (-l) #1507

d.legend: handle 0 for log (-l) #1507

HuidaeCho commented Apr 6, 2021

HuidaeCho commented Apr 7, 2021

HuidaeCho commented Apr 7, 2021 •

edited

Loading

neteler commented Apr 11, 2021

HuidaeCho commented Apr 16, 2021

HuidaeCho commented Apr 16, 2021

neteler commented Apr 19, 2021

neteler commented Apr 19, 2021

veroandreo commented Apr 22, 2021

veroandreo commented Apr 22, 2021 •

edited

Loading

HuidaeCho commented Apr 23, 2021 •

edited

Loading

HuidaeCho commented Apr 23, 2021

HuidaeCho commented Apr 23, 2021

neteler commented Feb 18, 2023

petrasovaa left a comment

petrasovaa Feb 20, 2023

petrasovaa Feb 20, 2023 •

edited

Loading

d.legend: handle 0 for log (-l) #1507

Are you sure you want to change the base?

d.legend: handle 0 for log (-l) #1507

Conversation

HuidaeCho commented Apr 6, 2021

HuidaeCho commented Apr 7, 2021

HuidaeCho commented Apr 7, 2021 • edited Loading

neteler commented Apr 11, 2021

HuidaeCho commented Apr 16, 2021

HuidaeCho commented Apr 16, 2021

neteler commented Apr 19, 2021

neteler commented Apr 19, 2021

Negative values

Negative to positive

0 to positive

veroandreo commented Apr 22, 2021

veroandreo commented Apr 22, 2021 • edited Loading

HuidaeCho commented Apr 23, 2021 • edited Loading

HuidaeCho commented Apr 23, 2021

HuidaeCho commented Apr 23, 2021

neteler commented Feb 18, 2023

petrasovaa left a comment

Choose a reason for hiding this comment

petrasovaa Feb 20, 2023

Choose a reason for hiding this comment

petrasovaa Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

HuidaeCho commented Apr 7, 2021 •

edited

Loading

veroandreo commented Apr 22, 2021 •

edited

Loading

HuidaeCho commented Apr 23, 2021 •

edited

Loading

petrasovaa Feb 20, 2023 •

edited

Loading