[GRASS-dev] [GRASS GIS] #392: backport G_is_c_null_value() to devbr6

GDAL · December 7, 2008, 1:41pm

#392: backport G_is_c_null_value() to devbr6
---------------------+------------------------------------------------------
Reporter: hamish | Owner: grass-dev@lists.osgeo.org
Type: task | Status: new
Priority: major | Milestone: 6.4.0
Component: default | Version: svn-develbranch6
Keywords: | Platform: All
Cpu: All |
---------------------+------------------------------------------------------
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).
}}}

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 15, 2008, 1:38am

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: major | Milestone: 6.4.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Comment (by 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:1>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 15, 2008, 4:25am

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: major | Milestone: 6.4.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Comment (by glynn):

Replying to [comment:1 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:2>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 17, 2008, 6:59am

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: major | Milestone: 6.4.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Comment (by hamish):

Replying to [comment:2 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

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/392#comment:3>
GRASS GIS <http://grass.osgeo.org>

GDAL · December 17, 2008, 4:19pm

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: major | Milestone: 6.4.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Comment (by glynn):

Replying to [comment:3 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.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/392#comment:4>
GRASS GIS <http://grass.osgeo.org>

Paul_Kelly · December 17, 2008, 8:11pm

GRASS GIS wrote:

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
Reporter: hamish | Owner: grass-dev@lists.osgeo.org
Type: task | Status: new Priority: major | Milestone: 6.4.0 Component: default | Version: svn-develbranch6 Resolution: | Keywords: Platform: All | Cpu: All ----------------------+-----------------------------------------------------
Comment (by hamish):

Replying to [comment:2 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?

Just a little comment about I'm not sure about the logic of intentionally planning to change a release candidate. To me, "release candidate" implies "this might be identical to the final release if we don't find any last minute bugs". If we're intentionally planning another change before the release it isn't really a release candidate after all. Maybe a pre-release would be a better name, e.g. 6.4.0pre1.

But I realise that doesn't really help anything towards the problem with slow and delayed releases. Personally I prefer more frequent releases even if there are known bugs, but it seems very hard to avoid the temptation to get last minute improvements into each release, which then necessarily delays the release while the last minute changes are tested, and things just tend to slip back further. More frequent releases may be a solution to that but I'm not sure.

Paul

neteler · December 18, 2008, 7:42am

On Wed, Dec 17, 2008 at 9:11 PM, Paul Kelly
<paul-grass@stjohnspoint.co.uk> wrote:

#392: backport G_is_c_null_value() to devbr6

Comment (by 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?

Since there are no comments there does not seem to be a bug
in GRASS 6.x.
So: if it ain't broken, don't fix it.

Just a little comment about I'm not sure about the logic of intentionally
planning to change a release candidate. To me, "release candidate" implies
"this might be identical to the final release if we don't find any last
minute bugs". If we're intentionally planning another change before the
release it isn't really a release candidate after all.

I agree. But apparently the problem isn't there and we can now
create the release branch.

Markus

Paul_Kelly · December 18, 2008, 1:08pm

On Thu, 18 Dec 2008, Markus Neteler wrote:

On Wed, Dec 17, 2008 at 9:11 PM, Paul Kelly
<paul-grass@stjohnspoint.co.uk> wrote:

#392: backport G_is_c_null_value() to devbr6

Comment (by 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?

Since there are no comments there does not seem to be a bug
in GRASS 6.x.
So: if it ain't broken, don't fix it.

That's true; I agree. It's definitely not a bug; just a change for potential future compatibility with invalid raster files so we don't need it now.

Just a little comment about I'm not sure about the logic of intentionally
planning to change a release candidate. To me, "release candidate" implies
"this might be identical to the final release if we don't find any last
minute bugs". If we're intentionally planning another change before the
release it isn't really a release candidate after all.

I agree. But apparently the problem isn't there and we can now
create the release branch.

Sounds good - is renaming nviz_cmd the only issue (actually a minor issue since we still have the old nviz in 6.4) left then?

Paul

Hamish · December 19, 2008, 3:35am

MN:

> Since there are no comments there does not seem to be a bug
> in GRASS 6.x.
> So: if it ain't broken, don't fix it.

PK:

That's true; I agree. It's definitely not a bug; just a change for
potential future compatibility with invalid raster files so
we don't need it now.

just to note that currently FCELL,DCELL checks do it the "new way" of
if (x != x), while CELL still checks it the old way. ie F,D got
backported but CELL didn't.

the commit log said check if both all 0s and if nan, but the check is just
for nan. Is it the case that for all modern systems (IEEE FP's) the two
are identical?

see ya,
Hamish

neteler · December 19, 2008, 8:49am

On Thu, Dec 18, 2008 at 2:08 PM, Paul Kelly
<paul-grass@stjohnspoint.co.uk> wrote:

On Thu, 18 Dec 2008, Markus Neteler wrote:

I agree. But apparently the problem isn't there and we can now
create the release branch.

Sounds good - is renaming nviz_cmd the only issue (actually a minor issue
since we still have the old nviz in 6.4) left then?

Call it nviz2?

We are running out of time with respect to the QGIS 1.0 release.

Markus

martinl · December 19, 2008, 11:08am

Hi,

2008/12/19 Markus Neteler <neteler@osgeo.org>:

On Thu, Dec 18, 2008 at 2:08 PM, Paul Kelly
<paul-grass@stjohnspoint.co.uk> wrote:

On Thu, 18 Dec 2008, Markus Neteler wrote:

I agree. But apparently the problem isn't there and we can now
create the release branch.

Sounds good - is renaming nviz_cmd the only issue (actually a minor issue
since we still have the old nviz in 6.4) left then?

Call it nviz2?

nviz -> TCL/TK GUI
nviz2 -> cmd line based module

seems to be confusing for me...

I vote for d.3d.

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

martinl · December 19, 2008, 11:09am

Hi,

2008/12/19 Martin Landa <landa.martin@gmail.com>:

Call it nviz2?

nviz -> TCL/TK GUI
nviz2 -> cmd line based module

seems to be confusing for me...

I vote for d.3d.

or we can leave it as nviz_cmd and go ahead to 6.4.0RC1...

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

martinl · December 19, 2008, 12:03pm

Hi,

2008/12/19 Martin Landa <landa.martin@gmail.com>:

Call it nviz2?

nviz -> TCL/TK GUI
nviz2 -> cmd line based module

seems to be confusing for me...

I vote for d.3d.

or we can leave it as nviz_cmd and go ahead to 6.4.0RC1...

considering d.nviz which should be also renamed (in 7.0)

6.4

d.nviz
nviz_cmd -> d.3d

7.0

d.nviz -> d.3d.fly
nviz_cmd -> d.3d

?

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

Glynn_Clements1 · December 19, 2008, 2:11pm

Hamish wrote:

MN:
> > Since there are no comments there does not seem to be a bug
> > in GRASS 6.x.
> > So: if it ain't broken, don't fix it.
PK:
> That's true; I agree. It's definitely not a bug; just a change for
> potential future compatibility with invalid raster files so
> we don't need it now.

just to note that currently FCELL,DCELL checks do it the "new way" of
if (x != x), while CELL still checks it the old way. ie F,D got
backported but CELL didn't.

the commit log said check if both all 0s

All-ones. All-zeroes is (positive) zero.

and if nan, but the check is just
for nan. Is it the case that for all modern systems (IEEE FP's) the two
are identical?

According to IEEE-754, any value where the exponent is all-ones and
the mantissa is non-zero is a NaN. That includes the all-ones pattern,
but isn't limited to it.

However, you can't guarantee that operations such as assignment or
argument-passing preserve the exact bit pattern. If you pass a NaN as
a function argument, the parameter variable will be NaN, but it might
not be the exact same bit pattern.

Also, you can't guarantee that NaNs generated by the FPU (e.g. 0.0/0)
*won't* be the all-ones bit pattern, i.e. you can't guarantee the
ability to distinguish between "GRASS nulls" and NaNs arising from FP
arithmetic.

Finally, if there's an architecture where the all-ones pattern isn't
NaN, then it's a defined value, which means that it can arise as a
result of normal FP arithmetic.

--
Glynn Clements <glynn@gclements.plus.com>

GDAL · February 9, 2009, 11:45pm

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: blocker | Milestone: 6.5.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Changes (by hamish):

* priority: major => blocker
* milestone: 6.4.0 => 6.5.0

Comment:

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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:5>
GRASS GIS <http://grass.osgeo.org>

GDAL · February 9, 2009, 11:49pm

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: blocker | Milestone: 6.5.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords:
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Comment (by 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:6>
GRASS GIS <http://grass.osgeo.org>

GDAL · February 18, 2009, 2:12am

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: minor | Milestone: 6.5.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords: NULL
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Changes (by hamish):

* priority: blocker => minor
* keywords: => NULL

Comment:

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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:7>
GRASS GIS <http://grass.osgeo.org>

GDAL · February 18, 2009, 3:27am

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: new
  Priority: minor | Milestone: 6.5.0
Component: default | Version: svn-develbranch6
Resolution: | Keywords: NULL
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Comment (by 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:8>
GRASS GIS <http://grass.osgeo.org>

GDAL · February 22, 2009, 8:57am

#392: backport G_is_c_null_value() to devbr6
----------------------+-----------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: task | Status: closed
  Priority: minor | Milestone: 6.5.0
Component: default | Version: svn-develbranch6
Resolution: fixed | Keywords: NULL
  Platform: All | Cpu: All
----------------------+-----------------------------------------------------
Changes (by hamish):

* status: new => closed
* resolution: => fixed

Comment:

backported to develbranch_6 in r36024.

closing ticket.

Hamish

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/392#comment:9>
GRASS GIS <http://grass.osgeo.org>