d.legend: handle 0 for log (-l) by HuidaeCho · Pull Request #1507 · OSGeo/grass

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

+                            "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

+                    /* 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.

echoix · 2025-08-10T00:37:56Z

Solved conflicts

echoix · 2026-10-03T14:41:00Z

I've made an agent try this old PR out. One bigger issue: the hard-coded eps = 1.0 fallback gives a degenerate legend whenever the raster maximum is <= 1 (e.g. fractions or probabilities).

With a 0..1 raster and d.legend -c -l I only get the "assuming a positive min cell value of 1" warning, exit 0, and the legend below (one solid bar, no labels). For a 0..0.45 raster the scale is inverted (eps_min > dmax), and at dmax == 1 the log10(dmax) - log10(eps_min) denominator is 0.

Other notes

The dmax <= 0 branches (color loop, labels, tick loop, tstep loop) are unreachable now that there is a fatal error for that case, and the XXX comments say the author is unsure whether to keep them. Drop them.
eps_min is uninitialized when -l is not used; it is only safe because it is read under log_sc.
The warning says "discarding colors for cells <= 1", but with e.g. dmin = 0.2 the values between 0.2 and 1 are discarded too.
No update to d.legend.md/.html for the new log-scale behavior, and no test (a pytest in display/d.legend/tests/ for mixed and all-negative rasters would pin it down).
Checked on a build of this branch merged into current main: positive rasters are unchanged; a mixed raster warns and renders; an all-negative raster gives the new fatal error; label_values=-1,... with -l skips the non-positive tick with a warning.

echoix · 2026-10-03T14:41:16Z

    struct FPRange fprange, render_range;
    CELL min_ind, max_ind;
-    DCELL dmin, dmax, val;
+    DCELL dmin, dmax, val, eps_min, eps = 1.0;


A fixed eps = 1.0 floor breaks the legend for any raster with dmax <= 1 (e.g. 0..1 gives a single-color bar with no labels, and 0..0.45 gives an inverted scale because eps_min > dmax). Derive the floor from the data (a fraction of dmax, or the smallest positive cell value), or fail with an error when no sensible floor exists.

Also, eps_min is uninitialized when -l is not used. It is only read under log_sc, but initializing it would be safer.

echoix · 2026-10-03T14:41:16Z

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


The warning says colors for cells <= 1 are discarded, but with 0 < dmin < 1 (e.g. 0.2) everything below 1 is discarded too, not just non-positive cells. And when dmax <= eps, nothing tells the user the legend is wrong: it exits 0 and draws a degenerate legend.

echoix · 2026-10-03T14:41:16Z

                if (log_sc) { /* logarithmic scale */
                    num = k / lleg;
-                    val = dmin * pow(dmax / dmin, num);
+                    /* XXX: for log scale, if the entire raster is not


dmax <= 0 is already rejected by the fatal error at lines 326-328, so this XXX block and the dmax <= 0 ? dmax : ... on line 416 are dead code. I'd drop them and keep the fatal error, which also settles the question in the comment.

echoix · 2026-10-03T14:41:16Z

        /* Format text */
-        if (!fp) { /* cut down labelnum so they don't repeat */
+        if (fp) {
+            if (log_sc && dmax <= 0) /* label min and max only */


Same unreachable dmax <= 0 handling here (steps = 2), and in the matching branches at lines 558, 594, 629 and 761-770 (the k == 0 / t_start == 0 special cases and the 1.5 * t_step loop bound). They can all go.

neteler · 2026-10-07T17:26:20Z

I've made an agent try this old PR out.

Thanks for looking into this, old, yet (in my view) relevant PR.

echoix · 2026-10-07T17:28:11Z

To make it go forward, a decision on the expected behaviour needs to be made, until then, it'll take another 6 years?

petrasovaa · 2026-10-09T04:24:42Z

I am working on this. We should probably take approach from #1937.

petrasovaa · 2026-10-27T02:20:26Z

Since #8060 was merged that solves this problem, I will close this.

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 3 commits June 25, 2024 23:36

Merge branch 'main' into d_legend_log

6c271f7

Merge branch 'main' into d_legend_log

74f0487

Merge branch 'main' into pr/HuidaeCho/1507

946934e

Merge branch 'main' into d_legend_log

e4fb492

echoix reviewed Oct 3, 2026

View reviewed changes

petrasovaa mentioned this pull request Oct 13, 2026

d.legend: Support logarithmic scale for ranges with non-positive values #8060

Merged

petrasovaa closed this Oct 27, 2026

Conversation

HuidaeCho commented Apr 6, 2021

Uh oh!

HuidaeCho commented Apr 7, 2021

Uh oh!

HuidaeCho commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neteler commented Apr 11, 2021

Uh oh!

HuidaeCho commented Apr 16, 2021

Uh oh!

HuidaeCho commented Apr 16, 2021

Uh oh!

neteler commented Apr 19, 2021

Uh oh!

neteler commented Apr 19, 2021

Negative values

Negative to positive

0 to positive

Uh oh!

veroandreo commented Apr 22, 2021

Uh oh!

veroandreo commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HuidaeCho commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HuidaeCho commented Apr 23, 2021

Uh oh!

HuidaeCho commented Apr 23, 2021

Uh oh!

neteler commented Feb 18, 2023

Uh oh!

petrasovaa left a comment

Choose a reason for hiding this comment

Uh oh!

petrasovaa Feb 20, 2023

Choose a reason for hiding this comment

Uh oh!

petrasovaa Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echoix commented Aug 10, 2025

Uh oh!

echoix commented Oct 3, 2026

Uh oh!

echoix Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

echoix Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

echoix Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

echoix Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

neteler commented Oct 7, 2026

Uh oh!

echoix commented Oct 7, 2026

Uh oh!

petrasovaa commented Oct 9, 2026

Uh oh!

petrasovaa commented Oct 27, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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