[GRASS-dev] [GRASS GIS] #1576: r.in.poly broken by window split

#1576: r.in.poly broken by window split
------------------------------------------+---------------------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, Rast_set_window() | Platform: All
      Cpu: All |
------------------------------------------+---------------------------------
Hi,

r.in.poly was missed during the split window transition (r42876) and now
exits with an error. (previously there was a suppressed-by-the-module
warning)
{{{
ERROR: Output window changed while maps are open for write
}}}

a simple change in the row paging code of Rast_set_window() to
Rast_set_output_window() in r.in.poly/raster.c didn't help; any advice on
how the input/output split windows now work? the doxygen lib fn comments
briefly explain the what, but not the why of the design or how you're
supposed to use it.

thanks,
Hamish

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/1576&gt;
GRASS GIS <http://grass.osgeo.org>

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------
Changes (by hamish):

* cc: stefansylla@… (added)
  * keywords: r.in.poly, Rast_set_window() => r.in.poly, i.topo.corr,
               Rast_set_window(),

Comment:

i.topo.corr in grass7 addons too.

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

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------

Comment(by mmetz):

Replying to [comment:1 hamish]:
> i.topo.corr in grass7 addons too.

Both r.in.poly and i.topo.corr should be fixed now.

Markus M

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

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------

Comment(by glynn):

Replying to [ticket:1576 hamish]:

> any advice on how the input/output split windows now work?

The input window determines the number of rows and columns for
Rast_get_row() etc, i.e. the valid range for the "row" parameter, and the
number of elements in the "buf" array.

The output window determines the number of rows and columns for
Rast_put_row() etc, i.e. the number of times it should be called ("put"
operations don't have a "row" parameter) and the number of elements in the
"buf" array.

For most modules, the input and output windows should be the same, but
e.g. r.resamp.* will typically use split windows.

The rationale was that the old mechanism was inefficient. It would change
the window twice for each row of output (set "read" window, read rows, set
"write" window, write row). Each change caused the column mapping to be
recalculated for each input map. With split windows, there are separate
windows for input maps and output maps, eliminating the need to change
back and forth between input and output windows. The column mapping is
calculated when a map is opened and never changes. Attempting to change
the input window while any input maps are open generates an error, as does
attempting to change the output window while any output maps are open.

Also, certain functions assume the existence of a single window, e.g.
Rast_get_window(), Rast_window_{rows,cols}(). These functions still exist,
and work so long as the windows aren't split (i.e. neither
Rast_set_input_window() nor Rast_set_output_window() have been called, or
the windows have since been joined by calling Rast_set_window()), but will
generate an error if the windows are split. If the windows are split, the
code must use e.g. Rast_get_input_window(), Rast_input_window_rows() etc
to read the appropriate window.

The "split window" design eliminates the most common reason for changing
the window while maps are open, although there may be cases it doesn't
cover (e.g. reading multiple input maps at different resolutions won't
work). If we need to support that, there are a number of possible
solutions.

One is to store the current window in the fileinfo structure whenever the
column mapping is created, check that it matches the current window
whenever Rast_read_row() etc is called, and either re-generate it or
generate an error if it differs. This avoids having to needlessly re-
calculate the column mapping for all input maps on every set-window
operation, but requires a window comparison (which probably needs some
tolerance for comparing floating-point fields) for each get-row operation.

Another is to allow each map to have a separate window, set from the
current window when the map is opened. The main drawback is that
Rast_window_rows() etc become meaningless. We would need to add yet
another "level" of window splitting, so you'd have: single window,
read/write windows, per-map windows.

As usual, the main problem isn't implementation, but design and ensuring
that existing code doesn't silently break (the current implementation
should ensure that any breakage results in a fatal error).

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/1576#comment:3&gt;
GRASS GIS <http://grass.osgeo.org>

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------

Comment(by neteler):

Please add a synthesis in lib/raster/rasterlib.dox

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/1576#comment:4&gt;
GRASS GIS <http://grass.osgeo.org>

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------

Comment(by neteler):

Replying to [comment:4 neteler]:
> Please add a synthesis in lib/raster/rasterlib.dox

Done in r59331 (verification in SVN recommended).

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/1576#comment:5&gt;
GRASS GIS <http://grass.osgeo.org>

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------

Comment(by hamish):

Replying to [comment:2 mmetz]:
> Both r.in.poly and i.topo.corr should be fixed now.

Hi,

fyi I still get the "ERROR: Output window changed while ..." error with
r.in.poly with the latest trunk. I used the example from the man page
piped through stdin.

thanks,
Hamish

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/1576#comment:6&gt;
GRASS GIS <http://grass.osgeo.org>

#1576: r.in.poly broken by window split
--------------------------------------------------------+-------------------
Reporter: hamish | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Keywords: r.in.poly, i.topo.corr, Rast_set_window(), | Platform: All
      Cpu: All |
--------------------------------------------------------+-------------------

Comment(by hamish):

{{{
g.region e=591450 w=591300 n=492660 s=492600 -p

r.in.poly in=- out=test1234 << EOF
A
    591316.80 4926455.50
    591410.25 4926482.40
    591434.60 4926393.60
    591341.20 4926368.70
= 42 stadium
EOF
}}}

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/1576#comment:7&gt;
GRASS GIS <http://grass.osgeo.org>

#1576: r.in.poly broken by window split
---------------------+------------------------------------------------------
  Reporter: hamish | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 7.0.0
Component: Raster | Version: svn-trunk
Resolution: fixed | Keywords: r.in.poly, i.topo.corr, Rast_set_window(),
  Platform: All | Cpu: All
---------------------+------------------------------------------------------
Changes (by wenzeslaus):

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

Comment:

I don't get any error with the code posted and G7:r.in.poly changed quite
a bit. Closing the ticket as fixed. Reopen if you are still getting
`...Output window changed while...` and please submit
[http://grass.osgeo.org/grass71/manuals/libpython/gunittest_testing.html a
test] which shows it.

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/1576#comment:8&gt;
GRASS GIS <http://grass.osgeo.org>