r.in.gdal & others: -e must only modify the current mapset's WIND file

[hamish]

due notice...

r.in.gdal, r.external, v.in.ogr in all branches:

The -e flag must only extend the current mapset's WIND, not the default one. Writing outside of the current mapset is completely forbidden!

Not even g.region lets you do this; if you want to update the DEFAULT_WIND you have to own and be in PERMANENT already. Not doing so breaks the multi-user model. The only time writing to DEFAULT_WIND is allowed is if you are creating a new location; AFAICT v.in.lidar in trunk acts that way.

(n.b. i.rectify is a historical anomaly)

Hamish

[hamish]

done in trunk and devbr6 with r49733,4.

awaiting testing before backporting to 6.4svn.

Hamish

[neteler]

Replying to hamish:

done in trunk and devbr6 with r49733,4.

awaiting testing before backporting to 6.4svn.

I am not at all convinced that this change in known behaviour should reach GRASS 6.4. AFAIK the functionality is there on purpose...

[hamish]

Replying to neteler:

I am not at all convinced that this change in known behaviour should reach GRASS 6.4.

my take on it--

backwards compatibility is allowed to be broken in cases where the previous behaviour was clearly in error and harming the user. in this case we have the competing interests of conserving expected behaviour (warts and all) versus the fact that the previous behaviour was is serious violation of the rule where modules are only allowed to make modifications within the current mapset. to allow that risks mayhem in a classroom/research team multi-user environment with a shared PERMANENT [can't trust file system write permissions to save us].

(modifying newly created locations/mapsets outside of the current one is of course ok -- but only during the process of creating them)

the decision of which of those two interests is more important is based on how bad the design bug was and how core or peripheral the feature change will be. there's no correct answer, just a judgment call. either way we win somehow, either way we lose somehow.

In this case I would not be too surprised if someone had a processing script expecting the old behaviour, which is quite unfortunate, but for my 2c I think the gross violation of g.access rules is the worse of the two options.

certainly if we decide to backport this to 6.4 it should feature prominently in the release notes.

AFAIK the functionality is there on purpose...

what was it? I mean versus having the module change the current WIND file only? If it is for r.in.gdal & v.in.ogr & newly created locations it's already taken care of, because G_make_location() takes &cellhd as one of its function argument and so the new DEFAULT_WIND and WIND are already set to match the import map's bounds.

best, Hamish

[hamish]

... for the case of a location created before you knew what the default bounds should be, & so you are left with n=1 s=0 e=1 w=0 in DEFAULT_WIND and you want some way to easily update it (restarting in PERMANENT just to fix that is a pain), consider that hiding that functionality in r.in.gdal & co is a bit weird for something needing a general solution.

I could see a small "g.region.update_default" addon helper script which ran g.mapset to temporarily switch into PERMANENT (with g.mapset's appropriate locking tests) run g.region -s, then switch back to the current mapset, but I don't think such a thing should be part of the main distribution.

perhaps a better solution to that problem is a pop-up tool on the startup mapset selection GUI which for the current location gave you a list of available mapsets in the same location and let you quickly reset the default region from the one you select. this still requires you to remember to do it at startup; but maybe the startup GUI could scan for 0-1,0-1 default regions at startup and color the PERMANENT text orange to remind you that it needs doing?

shrug, Hamish

[neteler]

When you create a new location (e.g. location wizard) without defining the boundaries, you obtain a 0-1 sized location. The use these import tools to set the DEFAULT_WIND (not WIND) using the now deleted flag.

That was the relevant purpose which I still want to use in future (on command line, in one step). I disagree that this functionality has been removed with a discussion time of less than 48hs.

[neteler]

I suggest to revert the changes and reinstate the original behaviour with the limitation that DEFAULT_WIND can only be modified if the user is in PERMANENT (warning + only WIND change otherwise).

[glynn]

Replying to hamish:

backwards compatibility is allowed to be broken in cases where the previous behaviour was clearly in error and harming the user.

Which doesn't apply here, IMHO.

Updating the default region rather than the current region was clearly intentional. The default window provides information about the data in the location. The current window is an operational parameter.

I'm not sure that having a large number of r.in.* modules, each having the ability to update the default region is the best approach. Having the modules update the current region then using an extra "g.region -s" step may be preferable.

OTOH, it's not unreasonable to ignore the locking issue when changing the default window, as the default window shouldn't be used for much beyond "g.region -d". Also, it should be sufficient to have write permission for the DEFAULT_WIND file; it shouldn't be necessary to own the containing mapset (that requirement exists mainly to deal with issues relating to subdirectories, which don't apply here).

[hamish]

Replying to neteler:

When you create a new location (e.g. location wizard) without defining the boundaries, you obtain a 0-1 sized location. The use these import tools to set the DEFAULT_WIND (not WIND) using the now deleted flag. That was the relevant purpose which I still want to use in future (on command line, in one step).

(see my comments about that in comment:4)

2c: It's a common problem; locating on disk + importing a geofile is a bit of a round-about way of solving it. It would be useful for having a general solution; if it belongs anywhere it belongs in g.region, and that already (rightly IMO) kicks you out with a "you need to be in PERMANENT" error. ..thus the g.mapset wrapper script & gui-startup suggestions for a more automated solution.

bug? if 0-1,0-1, and new imported map is like s=456000 n=457000, watch def_wind.south:

    /* -------------------------------------------------------------------- */
    /*      Extend current window based on dataset.                         */
    /* -------------------------------------------------------------------- */
    if (flag_e->answer) {
        G_get_default_window(&def_wind);

        def_wind.north = MAX(def_wind.north, cellhd.north);
        def_wind.south = MIN(def_wind.south, cellhd.south);
        def_wind.west = MIN(def_wind.west, cellhd.west);
        def_wind.east = MAX(def_wind.east, cellhd.east);

        def_wind.rows = (int)ceil((def_wind.north - def_wind.south)
                                  / def_wind.ns_res);
        def_wind.south = def_wind.north - def_wind.rows * def_wind.ns_res;

        def_wind.cols = (int)ceil((def_wind.east - def_wind.west)
                                  / def_wind.ew_res);
        def_wind.east = def_wind.west + def_wind.cols * def_wind.ew_res;

        G__put_window(&def_wind, "../PERMANENT", "DEFAULT_WIND");
    }

0 is the MIN() of 0,456000, so def_wind.south stays set to 0 after the first call to it, so then doesn't it corrupt the def_wind.rows calculation?? same is true for def_wind.west. If so how has it survived for years like that without anyone noticing?!

g.region -s rast=$map@original_mapset is looking safer..

(n.b. conflicting code comment ;-)

I suggest to revert the changes and reinstate the original behaviour with the limitation that DEFAULT_WIND can only be modified if the user is in PERMANENT (warning + only WIND change otherwise).

try r49745. If you like it we can update the help pages and sync with the other modules. (I'll leave the question of what to do in v.in.lidar up to Markus M)

I am not a big fan of operations that have special meaning based on context, but at least this way mostly does what the user wants most of the time: fails safe-ish.

The use case I had been considering was not 01/01 but saving the step of "g.region rast=newly_imported_map" and/or the "why do I get a blank display even though the import seemed to go ok?" FAQ.

I disagree that this functionality has been removed with a discussion time of less than 48hs.

fair enough; I'd considered this a priority for inclusion in 6.4.2, and with no comments on it so far I didn't want to stall a release for a week waiting. and I did say "due notice..." :-)

Glynn:

The default window provides information about the data in the location.

I guess this flag is "grow the current window" not "replace the current window", and in general you'd like to grow your location definition outwards from your first map as time goes on and your number of maps grow.

Hamish

[hamish]

Hamish wrote:

bug?

i.e. in application of dealing with the 0,1/0,1 problem; the code is growing the region outwards exactly as it promised it would and is coded properly for doing that particular job.

[hamish]

we have a couple of choices on how it should work:

if not in PERMANENT:

• extend current WIND + newly imported map

if in PERMANENT:

• extend DEFAULT_WIND + newly imported map

then

• stop

or

• extend WIND + newly imported map

or

• replace WIND based on extended DEFAULT_WIND + new map

in all cases a message will be output stating what has happened.

so when you're in PERMANENT both WIND and DEFAULT_WIND would get updated. but then should WIND and DEFAULT_WIND expand independently, or is it reasonable to just write out the newly expanded DEFAULT_WIND and completely replace PERMANENT's WIND? (this is how 'r.in.gdal -e' is currently coded in trunk)

any preference on how this should work?

Hamish

[glynn]

Replying to hamish:

Updating WIND only makes sense if the next step is to copy WIND to somewhere more permanent (either DEFAULT_WIND or a named region).

This opens up the issue of whether individual mapsets should have a DEFAULT_WIND. Or, alternatively, whether "g.region -d" and "g.region -s" should be synonyms for e.g. "g.region region=default" and "g.region save=default" respectively (with the former falling back to ../PERMANENT/DEFAULT_WIND if no region named "default" exists).

[hamish]

Replying to glynn:

Updating WIND only makes sense if the next step is to copy WIND to somewhere more permanent (either DEFAULT_WIND or a named region).

IMO that choice is up to the user.. I don't know what makes sense to their task. The use-case I was thinking about was so they could save the g.region rast=new_map or g.region rast=new_map,old_map step after import & so not wonder why the screen is white when they run d.rast new_map after the import seemed to go ok. In that case explicitly saving the expanded region is reproducible and there's no reason to save a copy of the new map's region alone as it can be recreated with g.region rast= whenever needed.

This opens up the issue of whether individual mapsets should have a DEFAULT_WIND. Or, alternatively, whether "g.region -d" and "g.region -s" should be synonyms for e.g. "g.region region=default" and "g.region save=default" respectively (with the former falling back to ../PERMANENT/DEFAULT_WIND if no region named "default" exists).

this is an interesting idea, it sounds useful and flexible & I'd support following it up in trunk.

but for the immediate task of 6.4.2, how should we proceed? i.e. is everyone happy with the current way of r.in.gdal in trunk? (expands DEFAULT_WIND and WIND if in PERMANENT; sets WIND if not in permanent; a message describing what happened happens in all cases) if so we can sync with the other modules & branches. if not what do y'all suggest?

Hamish

[hamish]

Hamish:

The use-case I was thinking about was so they could save the g.region rast=new_map [...] step

a bit unclear of me to use the word "save" there: i.e. so they could skip that step, nothing to do with save= to disk.

[neteler]

Replying to hamish:

but for the immediate task of 6.4.2, how should we proceed? i.e. is everyone happy with the current way of r.in.gdal in trunk? (expands DEFAULT_WIND and WIND if in PERMANENT; sets WIND if not in permanent; a message describing what happened happens in all cases) if so we can sync with the other modules & branches.

Yes, please sync.

[hamish]

Replying to glynn:

This opens up the issue of whether individual mapsets should have a DEFAULT_WIND. Or, alternatively, whether "g.region -d" and "g.region -s" should be synonyms for e.g. "g.region region=default" and "g.region save=default" respectively (with the former falling back to ../PERMANENT/DEFAULT_WIND if no region named "default" exists).

try the attached patch. perhaps the reading bit should go into G_get_default_wind() instead, but the nice thing about G_get_default_wind() is that it acts as a bit of a failsafe in case everything else gets corrupted, locally overriding it loses that.

Replying to neteler:

Yes, please sync.

done in trunk and devbr6. please test (ie 6.5svn as the source for the 6.4 backport). I haven't touched v.in.lidar, but I guess it should act the same as the others.

Hamish

[hamish]

patch to implement a per-mapset default region

[hamish]

Replying to hamish:

done in trunk and devbr6. please test (ie 6.5svn as the source for the 6.4 backport). I haven't touched v.in.lidar, but I guess it should act the same as the others.

AFAIK r.in.gdal, r.external, and v.in.ogr are ready for backport from 6.5svn, just needs testing.

Hamish

[neteler]

Hamish, can you either revert the changes and reinstate the original behaviour or backport with high priority? This issue is blocking for too much time now.

Markus

[hamish]

Replying to neteler:

Hamish, can you either revert the changes and reinstate the original behaviour or backport with high priority? This issue is blocking for too much time now.

sure, I'll be back somewhere with solid internet access tomorrow and do the backport then.

Hamish

[hamish]

r.in.gdal, r.external, and v.in.ogr changes backported to relbr64 in r50119. please test.

Hamish

[neteler]

Downgrading to critical.

[hamish]

tested in 6.4svn, everything seems to be working as expected.