backport G_is_c_null_value() to devbr6

[hamish]

I'm a bit overwhelmed at work right now, and Markus is busy too, so filing this as a task for 6.4 so that it doesn't get forgotten, & 'll get to it when I do.

Glynn wrote:

However, I suggest syncing G_is_c_null_value() (and possibly other
parts of null_val.c) to 7.0.

Doing this with "svn merge" isn't practical as some of the changes
were part of large-scale clean-ups (r34445, r34484).

The original code is over-complex as a result of trying to handle
arbitrary sizes for CELL, FCELL and DCELL. But I suspect that a lot
more than just null handling would break if those types weren't 4, 4
and 8 bytes respectively.

If you want to sync the whole file, you'll also need to sync the
prototypes in gisdefs.h, as some gratuitous "int" returns were changed
to "void". I don't think that anything actually checked the return
types, so it shouldn't be necessary to change anything else (unlike
e.g. G_get_window(), where many modules checked the return value even
though it would always be 1).

[hamish]

Hi Glynn,

I am trying to see which changes are needed here,

​http://trac.osgeo.org/grass/log/grass/trunk/lib/gis/null_val.c

you say r34445, r34484 are not needed, and r34446 is a fix to r34445, so I guess skip that too.

that leaves r34747 -- but things are different in 6.4. Possible patch attached, but I'm not really sure of the nature of the bug to know if it's right or not.

Hamish

[hamish]

possible patch for null_val.c bug

[glynn]

Replying to hamish:

I am trying to see which changes are needed here,

​http://trac.osgeo.org/grass/log/grass/trunk/lib/gis/null_val.c

you say r34445, r34484 are not needed, and r34446 is a fix to r34445, so I guess skip that too.

Actually r34445 is the main change which should be applied, but that requires both r34446 and r34747, but r34747 probably won't apply without r34484, but r34484 needs the corresponding changes to gisdefs.h, but only for null_val.c, unless you're going to apply r34484 to all 72 files.

that leaves r34747 -- but things are different in 6.4. Possible patch attached, but I'm not really sure of the nature of the bug to know if it's right or not.

No. "(CELL) &cellNullPattern" isn't valid. If you want to retain the existing byte arrays as the reference values, you would need to retain the existing code (or use memcmp).

I suggest completely replacing null_val.c with the 7.0 version, and editing gisdefs.h to match. Either that, or leave it as-is.

[hamish]

Replying to glynn:

I suggest completely replacing null_val.c with the 7.0 version, and editing gisdefs.h to match. Either that, or leave it as-is.

In that case, for the rc1 release I'd say leave it as is. It's too core a fn to mess with so close to release time.

For rc2, to completely replace null_val.c or not? I am still a bit unsure- is there actually a bug in the current devbr6 version or is the idea to keep the methods in sync to ease future maintenance?

Forward compatibility with grass7 raster maps is of course a good thing, if only on the binary data level (ie even if dir struct changes). Or are the updates helpful for r.external null cells?

note that r33717 from truck was already backported in r33752. (Make G_is_[fd]_null_value() check for any NaN, not just all-ones)

? Hamish

[glynn]

Replying to hamish:

For rc2, to completely replace null_val.c or not? I am still a bit unsure- is there actually a bug in the current devbr6 version or is the idea to keep the methods in sync to ease future maintenance?

It's just clean-up. The existing code is just unnecessarily complex: testing whether a value is equal to 0x80000000 by first ensuring that some other function has initialised a variable to that value then comparing the two byte-by-byte.

Actually, I was wrong about r34747 not applying without r34484. So you can use:

svn merge -c 34445,34446,r34747 https://svn.osgeo.org/grass/grass/trunk/lib/gis/null_val.c lib/gis/null_val.c

to merge everything except the prototype changes.

[hamish]

It is worse than I thought. Any module creating a map containing nulls in grass 6.5 currently generates a broken map.

G65> r.mapcalc 'nullmap = null()'
 100%

G65> r.info -r nullmap
min=-2147483648
max=-2147483648

G65> r.univar nullmap
 100%
total null and non-null cells: 2654802
total null cells: 0

Of the non-null cells:
----------------------
n: 2654802
minimum: 0
maximum: 0
range: 0
mean: 0
mean of absolute values: 0
standard deviation: 0
variance: 0
variation coefficient: nan %
sum: 0

If you read the above map in grass64 or grass7 you get the same r.info and r.univar output.

:-(

Hamish

[hamish]

and looking at an all-NULL map in GRASS 6.5 which was created in 6.4 or 7:

G65> r.univar nullmap7 
 100%
total null and non-null cells: 2654802
total null cells: 0

Of the non-null cells:
----------------------
n: 2654802
minimum: -2.14748e+09
maximum: -2.14748e+09
range: 0
mean: -2.14748e+09
mean of absolute values: 2.14748e+09
standard deviation: 0
variance: 0
variation coefficient: -0 %
sum: -5701143883677696

ouch.

Hamish

[hamish]

um, nevermind about the recent noise re. broken NULL support in GRASS 6.5. It was only due to local modifications at my end partially implementing this patch.

develbranch_6 and releasebranch_6_4 both still use the old way of testing for a NULL cell. Which takes the conversation back to:

Glynn:

I suggest completely replacing null_val.c with the 7.0 version, and editing gisdefs.h to match. Either that, or leave it as-is.

Hamish:

In that case, for the rc1 release I'd say leave it as is. It's too core a fn to mess with so close to release time.

For rc2, to completely replace null_val.c or not? I am still a bit unsure- is there actually a bug in the current devbr6 version or is the idea to keep the methods in sync to ease future maintenance?

Glynn:

It's just clean-up. The existing code is just unnecessarily complex:

...

So you can use:

svn merge -c 34445,34446,r34747 https://svn.osgeo.org/grass/grass/trunk/lib/gis/null_val.c lib/gis/null_val.c

overly complex to the point of significantly slowing everything down? If there is some opportunity to speed up raster processing by some tangible amount it may be worth it, otherwise I'd play it safe and do nothing. Probably would have to do some time trials to answer that:

### grass 6.5 ###
# spearfish
g.region rast=fields res=1 -p
  rows:       14000
  cols:       19000

time r.univar fields
  total null and non-null cells: 266000000
  total null cells: 101220000
-1-
real    0m41.798s
user    0m40.347s
sys     0m0.360s
-2-
real    0m42.245s
user    0m40.179s
sys     0m0.340s
-3-
real    0m42.601s
user    0m40.359s
sys     0m0.268s

### grass 7.0 ###
# spearfish
g.region rast=fields res=1 -p
time r.univar fields
-1-
real    0m36.288s
user    0m35.106s
sys     0m0.268s
-2-
real    0m36.079s
user    0m34.926s
sys     0m0.272s
-3-
real    0m36.274s
user    0m35.154s
sys     0m0.300s

So about 15% faster in GRASS 7. I don't know if all of that can be attributed to this change, but if likely then it would be very useful to backport it, at least to develbranch_6.

Hamish

[hamish]

combined patch for grass 6

[hamish]

Hamish wrote:

So about 15% faster in GRASS 7. I don't know if all of that can be attributed to this change, but if likely then it would be very useful to backport it, at least to develbranch_6.

Times for develbranch_6 with the patch (newly attached):

### grass 6.5 with patch ###
-1-
real    0m36.481s
user    0m35.190s
sys     0m0.320s
-2-
real    0m36.478s
user    0m35.318s
sys     0m0.256s
-3-
real    0m36.752s
user    0m35.246s
sys     0m0.400s

So, yes, this change does account for the bulk of the time difference. Unless there is any objection I will apply it to develbranch_6 soon, but not to releasebranch_6_4.

Hamish

[hamish]

backported to develbranch_6 in r36024.

closing ticket.

Hamish