Opened 16 years ago

Last modified 4 years ago

#70 new task

imagery modules: strip @mapset part

Reported by: hamish Owned by: grass-dev@…
Priority: major Milestone: 8.0.0
Component: Imagery Version: svn-trunk
Keywords: imagery Cc: Paula.Sankelo@…
CPU: Unspecified Platform: Unspecified

Description

Hi,

if you call i.target from a GUI and select a group it appends @mapset. If you run it like that it takes the full name literally and instead of updating the group's target it creates a new group of name "group@mapset". Either i.target or I_put_target() should check that if the @mapset part is given it refers to the current mapset, then strip off the @mapset part.

Otherwise you end up with

GRASS> g.list group

group
group@mapset
group@mapset@mapset

etc.

I changed the group option gisprompt from old,group,group to any,gr,gr. This makes the GUI group picker button go away, but perhaps the GUI picker button should be coded to remain in that case?

Or to solve this should we try changing the gisprompt to mapset,gr,gr so it calls G_ask_in_mapset()?

Hamish

Attachments (3)

imagery-find.patch (1.4 KB ) - added by glynn 16 years ago.
qualified name patch for I_find_* functions
imagery-mapset.patch (7.1 KB ) - added by glynn 16 years ago.
more extensive lib/imagery mapset patch
sigfile.diff (480 bytes ) - added by neteler 16 years ago.
fix for i.class

Download all attachments as: .zip

Change History (27)

in reply to:  description comment:1 by glynn, 16 years ago

Replying to hamish:

if you call i.target from a GUI and select a group it appends @mapset. If you run it like that it takes the full name literally and instead of updating the group's target it creates a new group of name "group@mapset". Either i.target or I_put_target() should check that if the @mapset part is given it refers to the current mapset, then strip off the @mapset part.

I changed the group option gisprompt from old,group,group to any,gr,gr. This makes the GUI group picker button go away, but perhaps the GUI picker button should be coded to remain in that case?

Or to solve this should we try changing the gisprompt to mapset,gr,gr so it calls G_ask_in_mapset()?

To solve this, i.group needs to be modified to handle the @mapset part correctly. That is the only course of action which can accurately be said to *solve* the issue.

There may be workarounds, but a workaround isn't the same thing as a solution.

And any function named G_ask_<something> shouldn't be used for anything. Those functions cause problems for GUI use (requiring a terminal), and will disappear the moment that 7.x comes into being.

comment:3 by hamish, 16 years ago

Cc: Paula.Sankelo@… added
Keywords: imagery added
Summary: i.target from GUI: strip @mapset partimagery modules: strip @mapset part

i.group, i.target modified in SVN to handle @mapset qualified names in r30802. i.class done in r30803.

As this is a systemic problem a better approach would be to improve the imagery libs to deal with that -- renaming bug and leaving it open.

I don't really use the imagery modules so I'm not sure what other modules are badly crippled from this. Proably most, but as mentioned above the way to tackle that is to fix it properly in the imagery lib fns.

e.g. listing maps contained in a group from another mapset should work.

Hamish

in reply to:  3 ; comment:4 by glynn, 16 years ago

Replying to hamish:

As this is a systemic problem a better approach would be to improve the imagery libs to deal with that -- renaming bug and leaving it open.

I have attached a patch which should fix the I_find_* functions to handle qualified names. Can someone test it?

by glynn, 16 years ago

Attachment: imagery-find.patch added

qualified name patch for I_find_* functions

by glynn, 16 years ago

Attachment: imagery-mapset.patch added

more extensive lib/imagery mapset patch

in reply to:  4 comment:5 by glynn, 16 years ago

Replying to glynn:

I have attached a more extensive version of the previous patch.

attachment:ticket:70:imagery-mapset.patch

This should cover all of the cases where filenames were being constructed manually.

There remains the issue that the imagery library can only handle groups in the current mapset, for both read and write. Most of that could be fixed by replacing G_mapset() with "". However, the I_find_* functions really need _new and _old versions, as (unlike the G_find_* functions) they don't accept a mapset argument.

comment:7 by neteler, 16 years ago

(i.group functional again).

I tried to use i.class, NC data set:

i.group mylsat7_2000 sub=mylsat7_2000 in=lsat7_2000_10,lsat7_2000_20,lsat7_2000_30,lsat7_2000_40,lsat7_2000_50,lsat7_2000_61,lsat7_2000_70,lsat7_2000_80

g.region rast=lsat7_2000_40 -p
d.mon x0
i.class map=lsat7_2000_40 group=mylsat7_2000 subgroup=mylsat7_2000 outsig=lsat7_2000_sig

RESULT SIGNATURE
WARNING: unable to create signature file lsat7_2000_sig for subgroup
         mylsat7_2000 of group mylsat7_2000
ERROR: Unable to open output signature file 'lsat7_2000_sig'

It seems to be an issue in lib/imagery/sigfile.c (devel_grass6 branch).

Markus

in reply to:  7 comment:8 by glynn, 16 years ago

Replying to neteler:

(i.group functional again).

> I tried to use i.class, NC data set:
> WARNING: unable to create signature file lsat7_2000_sig for subgroup
>          mylsat7_2000 of group mylsat7_2000
> ERROR: Unable to open output signature file 'lsat7_2000_sig'

It seems to be an issue in lib/imagery/sigfile.c (devel_grass6 branch).

Hopefully fixed in r31419 (trunk), r31420 (6.4).

comment:9 by neteler, 16 years ago

Possibly we need yet another one? Patch attached.

Markus

comment:10 by neteler, 16 years ago

Priority: minormajor

i.class is not yet working.

Markus

by neteler, 16 years ago

Attachment: sigfile.diff added

fix for i.class

comment:11 by neteler, 16 years ago

Fix applied for i.class (6.4 branch: r31439 and trunk: r31440). Working again.

Markus

comment:12 by neteler, 16 years ago

While i.class now works from cmd line, it still fails interactively:

GRASS 6.4.svn (nc_spm_07):~ > i.class

OPTION:   Name of raster map to be displayed
     key: map
  format: name
required: YES

Enter the name of an existing raster file
Enter 'list' for a list of existing raster files
Hit RETURN to cancel request
> lsat7_2002_40
<lsat7_2002_40>

OPTION:   Name of input imagery group
     key: group
  format: name
required: YES

Enter the name of an existing group file
Enter 'list' for a list of existing group files
Hit RETURN to cancel request
> mylsat7_2000
<mylsat7_2000>

OPTION:   Name of input imagery subgroup
     key: subgroup
  format: name
required: YES

Enter the name of an existing subgroup file
Enter 'list' for a list of existing subgroup files
Hit RETURN to cancel request
> list
<list>
----------------------------------------------


no subgroup files available in current mapset
----------------------------------------------

Enter the name of an existing subgroup file
Enter 'list' for a list of existing subgroup files
Hit RETURN to cancel request
> mylsat7_2000
<mylsat7_2000>

** mylsat7_2000 - not found **

Enter the name of an existing subgroup file
Enter 'list' for a list of existing subgroup files
Hit RETURN to cancel request
>  

I suspect a bug in grass/trunk/lib/imagery/find.c

Markus

in reply to:  12 comment:13 by glynn, 16 years ago

Replying to neteler:

While i.class now works from cmd line, it still fails interactively:

That's a separate issue, which was introduced in r24069.

<list>

no subgroup files available in current mapset

If you read that literally, it should give you a clue what's going on here.

I suspect a bug in grass/trunk/lib/imagery/find.c

Nope; it's a limitation of G_parser(), which cannot handle subgroups. Essentially, "old,subgroup,subgroup" isn't a valid ->gisprompt setting.

G_parser() itself knows nothing of the imagery library, and there is no way for individual modules to extend it. When G_parser() performs interactive input for an option with "old,<element>,<description>" as the ->gisprompt value, it just checks for the file in the <element> directory. Similarly, if you type "list", it just lists the <element> directory.

Well, mapsets don't have a "subgroup" directory, as subgroups belong to specific groups, not the mapset. Beyond that, G_parser() would have no way of knowing which group the subgroup belongs to (it cannot know that the value of the group= option indicates this).

Ultimately, you'll have to just remove the ->gisprompt setting, leaving the option as just a string.

comment:14 by neteler, 16 years ago

Indeed, removing for subgroup makes it pass. Fixes in:

GRASS 6.3.svn: r31465

GRASS 6.4.svn: r31464

GRASS 7.0.svn (trunk): r31463

Now i.class works again interactively, too.

Markus

comment:15 by neteler, 16 years ago

It seems that i.atcorr has some mapset related problems, too:

GRASS 6.4.svn (nc_spm_07):~ > g.gisenv
GISDBASE=/home/neteler/grassdata
LOCATION_NAME=nc_spm_07
MAPSET=sqlite
DEBUG=0
GRASS_GUI=text

GRASS 6.4.svn (nc_spm_07):~ > g.mapsets -p
sqlite PERMANENT

GRASS 6.4.svn (nc_spm_07):~ > i.atcorr -b iimg=lsat5_1987_60@landsat iscl=-6.978740,191.600 icnd=ddddd oimg=test_p112r056_b1.rad.atcorr oscl=0,255
G__open(r): mapset (sqlite) doesn't match xmapset (landsat)
G__open(r): mapset (sqlite) doesn't match xmapset (landsat)
WARNING: Unable to open header file for raster map
         <lsat5_1987_60@landsat@sqlite>
ERROR: Unable to retreive header dat for input image

GRASS 6.4.svn (nc_spm_07):~ > g.mapsets add=landsat

GRASS 6.4.svn (nc_spm_07):~ > i.atcorr -b iimg=lsat5_1987_60 iscl=-6.978740,191.600 icnd=ddddd oimg=test_p112r056_b1.rad.atcorr oscl=0,255
WARNING: Unable to open header file for raster map <lsat5_1987_60@sqlite>
ERROR: Unable to retreive header dat for input image

Markus

comment:16 by neteler, 16 years ago

OK, the i.atcorr mapset problems were unrelated. Fixed in r31614 and r31615.

Markus

comment:17 by neteler, 16 years ago

I have found a new related bug: -r of i.group fails.

g.gisenv set=DEBUG=3
GRASS 6.4.svn (nc_spm_07): > i.group -r mylsat7_2000 sub=mylsat7_2000 in=lsat7_2000_61
D3/3: remove_subgroup_files: ref_tmp.nfiles 8
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[0].name: lsat7_2000_10
D3/3: mapset neteler, ref_tmp.file[0].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[1].name: lsat7_2000_20
D3/3: mapset neteler, ref_tmp.file[1].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[2].name: lsat7_2000_30
D3/3: mapset neteler, ref_tmp.file[2].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[3].name: lsat7_2000_40
D3/3: mapset neteler, ref_tmp.file[3].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[4].name: lsat7_2000_50
D3/3: mapset neteler, ref_tmp.file[4].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[5].name: lsat7_2000_61
D3/3: mapset neteler, ref_tmp.file[5].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[6].name: lsat7_2000_70
D3/3: mapset neteler, ref_tmp.file[6].mapset: landsat
D3/3: tmp_name lsat7_2000_61, ref_tmp.file[7].name: lsat7_2000_80
D3/3: mapset neteler, ref_tmp.file[7].mapset: landsat
D1/3: writing subgroup REF
D2/3: G__home home = /home/neteler
WARNING: No raster map removed
i.group complete.

It compares the wrong mapsets here (G_mapset() should possibly not used on the left side, see function remove_subgroup_files() of main.c of i.group.

Markus

in reply to:  17 ; comment:18 by glynn, 16 years ago

Replying to neteler:

I have found a new related bug: -r of i.group fails.

Do r31775 and r31776 help?

G_mapset() returns a pointer to a static buffer. If you modify this buffer, you modify the return value for all subsequent G_mapset() calls. It should probably be change to "const char *" to catch this.

in reply to:  18 comment:19 by glynn, 16 years ago

Replying to glynn:

G_mapset() returns a pointer to a static buffer. If you modify this buffer, you modify the return value for all subsequent G_mapset() calls.

Oops; that's incorrect. G_mapset() actually checks that the value hasn't changed.

in reply to:  17 ; comment:20 by glynn, 16 years ago

Replying to neteler:

It compares the wrong mapsets here (G_mapset() should possibly not used on the left side, see function remove_subgroup_files() of main.c of i.group.

AFAICT, you should be able to use in=lsat7_2000_61@landsat to acheive the desired result.

Beyond that: unless "landsat" is in the mapset search path, this isn't a bug; you should have to explicitly specify the @landsat part.

OTOH, in the case where it is in the mapset search path, it's a limitation of the imagery library. The I_find_* functions only ever look for files in the current mapset (passing a qualified name to I_find_* will generate an error if the @mapset part doesn't specify the current mapset).

I didn't change this when I fixed the I_find_* functions because I can't easily determine if it will introduce potentially dangerous bugs (e.g. deleting the wrong file).

Unlike the G_find_* functions, the I_find_* functions don't have a mapset argument, so the caller doesn't get the option of passing G_mapset() instead of "" to prevent the use of other mapsets. Bear in mind that the same functions are used for both input (where other mapsets may be used) and output (where only the current mapset may be used).

in reply to:  20 comment:21 by neteler, 16 years ago

Replying to glynn:

Replying to neteler:

It compares the wrong mapsets here (G_mapset() should possibly not used on the left side, see function remove_subgroup_files() of main.c of i.group.

AFAICT, you should be able to use in=lsat7_2000_61@landsat to acheive the desired result.

this is working but not practical. I could have generated the group some years ago, for a normal user it's hard to find out this trick (yes, use -l and such but...).

Maybe: if a map appears only once in the list, remove it. Otherwise tell the user to indicate the mapset.

Beyond that: unless "landsat" is in the mapset search path, this isn't a bug; you should have to explicitly specify the @landsat part.

It's not obvious (see above).

OTOH, in the case where it is in the mapset search path, it's a limitation of the imagery library. The I_find_* functions only ever look for files in the current mapset (passing a qualified name to I_find_* will generate an error if the @mapset part doesn't specify the current mapset).

AFAIK the imagery library was written in the early days of GRASS, apparently improvements from libgis were never integrated.

I didn't change this when I fixed the I_find_* functions because I can't easily determine if it will introduce potentially dangerous bugs (e.g. deleting the wrong file).

Note that here we just remove a *reference* to a file (though I don't know about side effects of the used functions).

Unlike the G_find_* functions, the I_find_* functions don't have a mapset argument, so the caller doesn't get the option of passing G_mapset() instead of "" to prevent the use of other mapsets. Bear in mind that the same functions are used for both input (where other mapsets may be used) and output (where only the current mapset may be used).

Could the G_find_* functions be used instead?

Markus

comment:22 by neteler, 8 years ago

Milestone: 6.4.06.4.6

comment:23 by marisn, 5 years ago

Component: DefaultImagery
CPU: Unspecified
Milestone: 6.4.68.0.0
Platform: Unspecified
Type: defecttask

GRASS 7.8.0 will contain code allowing i.group to list content of groups in other mapsets. It also handles correctly creation of groups with maps from other mapsets (fully qualified map names). It was implemented in r73633 and r74107. See also: #3491

Still other imagery modules are working according to initial design – groups are useable only in the current mapset. Thus a new design should be proposed and modules changed to fit the new approach. I am not certain if such invasive changes should be done in GRASS 7.

One of options would be to handle groups like raster colours – in each mapset an override (shadowing) could be implemented. If a module requests write option to group, files are written into current mapset and thus changes are visible only in the current mapset. Downside – changes are accessible only from current mapset – it could be problematical for sig files.

Other approach would be to take signature files and classification results out from groups completely. There would be no big change in not writing classification output to group. With signature files it is more tricky, as new functions handling sig files then should be introduced or sig file handling should be shifted to the user (output to specified path/file instead of current mapset).

Note: See TracTickets for help on using tickets.