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

[HuidaeCho]

Attempt to address #1482

[HuidaeCho]

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

[HuidaeCho]

@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]

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]

* 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]

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]

* 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:

https://github.com/OSGeo/grass/issues/1482#issue-839905515

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

[neteler]

@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]

These are my tests with and without this PR:

• Without the PR:

1. positive values starting from 0.something (lots of small values some pretty high)

2. 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:

1. looks good - same as before the PR

2. 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]

@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]

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 (#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]

@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]

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

[neteler]

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