Open landam opened 5 years ago
Attachment from huhabla on 8 Apr 2010 14:23 UTC
https://trac.osgeo.org/grass/attachment/ticket/1031/parser_help.diff
Attachment from huhabla on 12 Apr 2010 13:09 UTC
https://trac.osgeo.org/grass/attachment/ticket/1031/parser.diff
Attachment from huhabla on 17 Apr 2010 22:46 UTC Simple examples of the new type system suggested by Glynn https://trac.osgeo.org/grass/attachment/ticket/1031/main.c
Attachment from huhabla on 17 Apr 2010 22:47 UTC Very simple prototype of the new type system suggested by Glynn https://trac.osgeo.org/grass/attachment/ticket/1031/type_system.h
Attachment from huhabla on 17 Apr 2010 22:48 UTC Very simple prototype of the new type system suggested by Glynn https://trac.osgeo.org/grass/attachment/ticket/1031/type_system.c
Comment by hamish on 9 Apr 2010 10:26 UTC most of what you want is already given by usage:
Usage:
r.buffer [-z] input=name output=name distances=value[,value,...]
[units=string] [--overwrite] [--verbose] [--quiet]
things in [square] brackets are optional (according to age-old unix convention (and prob ms-dos too))
the 'name' or 'value' on the right side of the equals sign, if not given a custom setting by the programmer with opt->key_desc, could perhaps be improved. see the tcltk module guis which are quite informative. I think we still have some work to do on this for the wx module guis (see also https://trac.osgeo.org/grass/ticket/886 for that).
Hamish
Comment by huhabla on 11 Apr 2010 13:14 UTC I am aware that some of informations i requests are given by the usage string. But IMHO it is more convenient and easier to understand (especially for command line beginner) to make these informations also available in the help description. My enhancement request is only related to the command line. It does not touch the automated gui generation.
The usage string does not always provide the convenient information if the parameter is of type raster, vector, volume, integer or float. In many modules input and output parameter are named related to the modules purpose, the information if its an input or output is only present if you read the help page.
Martin Landa currently renamed in several modules the input and output parameter to make clear which of them are inputs or outputs. I.e: elevation -> elevation_input, direction -> direction_output, but this is IMHO not a good idea. It is redundant information, especially if you are using the gui. In my opinion there is no need to rename the parameters, we can provide the information which the gui uses to make the data handling easy in the command line too.
Comment by @landam on 11 Apr 2010 13:35 UTC Replying to [comment:2 huhabla]:
I am aware that some of informations i requests are given by the usage string. But IMHO it is more convenient and easier to understand (especially for command line beginner) to make these informations also available in the help description. My enhancement request is only related to the command line. It does not touch the automated gui generation.
I think that the beginners usually do not use CLI at all. At least 'type' and 'required' are redundant info
input Name of input raster map
status: input
type: cell
required: yes
We could use key_desc
attribute for defining prompt type.
input=raster
instead of
input=name
The question is how to handle 'status' of the options.
The usage string does not always provide the convenient information if the parameter is of type raster, vector, volume, integer or float. In many modules input and output parameter are named related to the modules purpose, the information if its an input or output is only present if you read the help page.
Right, it could be improved by better key_desc
usage.
Martin Landa currently renamed in several modules the input and output parameter to make clear which of them are inputs or outputs. I.e: elevation -> elevation_input, direction -> direction_output, but this is IMHO not a good idea. It is redundant information, especially if you are using the gui.
Right, see [wiki:Grass7/NewFeatures#Renamedoptions Rename options]. Your idea about 'status' info seems to be better solution. If you prefer I can remove '_input/optput' from paramaters key string.
Comment by huhabla on 12 Apr 2010 12:17 UTC Replying to [comment:3 martinl]:
Replying to [comment:2 huhabla]:
I am aware that some of informations i requests are given by the usage string. But IMHO it is more convenient and easier to understand (especially for command line beginner) to make these informations also available in the help description. My enhancement request is only related to the command line. It does not touch the automated gui generation.
I think that the beginners usually do not use CLI at all. At least 'type' and 'required' are redundant info
input Name of input raster map status: input type: cell required: yes
We could use
key_desc
attribute for defining prompt type.input=raster
instead of
input=name
The question is how to handle 'status' of the options.
The usage string does not always provide the convenient information if the parameter is of type raster, vector, volume, integer or float. In many modules input and output parameter are named related to the modules purpose, the information if its an input or output is only present if you read the help page.
Right, it could be improved by better
key_desc
usage.
Thats a good idea. We could start modifying the default options in lib/gis/parser.c adding key_desc
. Although this information is already specified in the gisprompt
string ... we can use the convenient naming scheme used in gisprompt
for key_desc
.
Martin Landa currently renamed in several modules the input and output parameter to make clear which of them are inputs or outputs. I.e: elevation -> elevation_input, direction -> direction_output, but this is IMHO not a good idea. It is redundant information, especially if you are using the gui.
Right, see [wiki:Grass7/NewFeatures#Renamedoptions Rename options]. Your idea about 'status' info seems to be better solution. If you prefer I can remove '_input/optput' from paramaters key string.
Well this is only my humble opinion. In case other developers prefer the your naming schema i will accept it. Thats why i would like to discuss this topic.
I would personally prefer to remove '_input/optput' from paramaters key string and establish a combination of key_desc
and the additional "status" parameter in the command line. Maybe we can use a better name than "status" ... like "type"? Additionally we can hide the status parameter in case "input", "map" and "output" are used as key
strings?
Comment by huhabla on 12 Apr 2010 13:16 UTC I have attached a patch for lib/gis/parser_help.c and lib/gis/parser_stanndard_options.c which implements the discussed ideas.
Examples:
lib/gis> r.neighbors help
Description:
Makes each cell category value a function of the category values assigned to the cells around it, and stores new cell values in an output raster map layer.
Keywords:
raster
Usage:
r.neighbors [-ac] input=raster [selection=raster] output=raster
[method=string] [size=value] [title=phrase] [weight=filename]
[gauss=value] [quantile=value] [--overwrite] [--verbose] [--quiet]
Flags:
-a Do not align output with the input
-c Use circular neighborhood
--o Allow output files to overwrite existing files
--v Verbose module output
--q Quiet module output
Parameters:
input Name of input raster map
selection Name of an input raster map to select the cells which should be processed
type: input
output Name for output raster map
method Neighborhood operation
options: average,median,mode,minimum,maximum,stddev,sum,
variance,diversity,interspersion,quart1,quart3,perc90,
quantile
default: average
size Neighborhood size
default: 3
title Title of the output raster map
weight Text file containing weights
gauss Sigma (in cells) for Gaussian filter
quantile Quantile to calculate for method=quantile
options: 0.0-1.0
default: 0.5
lib/gis> r.buffer2 help
Description:
Creates a raster map layer showing buffer zones surrounding cells that contain non-NULL category values.
Keywords:
raster, buffer
Usage:
r.buffer2 [-z] input=raster output=raster distances=value[,value,...]
[units=string] [--overwrite] [--verbose] [--quiet]
Flags:
-z Ignore zero (0) data cells instead of NULL cells
--o Allow output files to overwrite existing files
--v Verbose module output
--q Quiet module output
Parameters:
input Name of input raster map
output Name for output raster map
distances Distance zone(s)
units Units of distance
options: meters,kilometers,feet,miles,nautmiles
default: meters
lib/gis> r.gwflow help
Description:
Numerical calculation program for transient, confined and unconfined groundwater flow in two dimensions.
Keywords:
raster, groundwater flow
Usage:
r.gwflow [-f] phead=raster status=raster hc_x=raster hc_y=raster
[q=raster] s=raster [recharge=raster] top=raster bottom=raster
output=raster [vx=raster] [vy=raster] [budget=raster] type=string
[river_bed=raster] [river_head=raster] [river_leak=raster]
[drain_bed=raster] [drain_leak=raster] dt=value [maxit=value]
[error=value] [solver=name] [--overwrite] [--verbose] [--quiet]
Flags:
-f Allocate a full quadratic linear equation system, default is a sparse linear equation system.
--o Allow output files to overwrite existing files
--v Verbose module output
--q Quiet module output
Parameters:
phead The initial piezometric head in [m]
type: input
status Boundary condition status, 0-inactive, 1-active, 2-dirichlet
type: input
hc_x X-part of the hydraulic conductivity tensor in [m/s]
type: input
hc_y Y-part of the hydraulic conductivity tensor in [m/s]
type: input
q Raster map water sources and sinks in [m^3/s]
type: input
s Specific yield in [1/m]
type: input
recharge Recharge map e.g: 6*10^-9 per cell in [m^3/s*m^2]
type: input
top Top surface of the aquifer in [m]
type: input
bottom Bottom surface of the aquifer in [m]
type: input
output The map storing the numerical result [m]
vx Calculate and store the groundwater filter velocity vector part in x direction [m/s]
type: output
vy Calculate and store the groundwater filter velocity vector part in y direction [m/s]
type: output
budget Store the groundwater budget for each cell [m^3/s]
type: output
type The type of groundwater flow
options: confined,unconfined
default: confined
river_bed The height of the river bed in [m]
type: input
river_head Water level (head) of the river with leakage connection in [m]
type: input
river_leak The leakage coefficient of the river bed in [1/s].
type: input
drain_bed The height of the drainage bed in [m]
type: input
drain_leak The leakage coefficient of the drainage bed in [1/s]
type: input
dt The calculation time in seconds
default: 86400
maxit Maximum number of iteration used to solver the linear equation system
default: 100000
error Error break criteria for iterative solvers (jacobi, sor, cg or bicgstab)
default: 0.0000000001
solver The type of solver which should solve the symmetric linear equation system
options: cg,pcg,cholesky
default: cg
Comment by glynn on 12 Apr 2010 21:49 UTC Replying to [comment:5 huhabla]:
I have attached a patch for lib/gis/parser_help.c and lib/gis/parser_stanndard_options.c which implements the discussed ideas.
The patch makes unnecessary formatting changes, which also contravene the GRASS formatting conventions. In general, formatting changes should be kept separate from other changes to make it easier to review the substance of the changes. But in this case, the formatting changes just shouldn't be there.
The updated output is unnecessarily verbose, which is a bad thing IMHO. The "required" and "multiple" status can already be determined from the "Usage:" section. Non-required options are listed in square brackets, while multiple options use the "option=value,..." convention. If you really want this format, it should be a separate option, e.g. --verbose-help.
Comment by @landam on 13 Apr 2010 13:56 UTC Replying to [comment:6 glynn]:
Replying to [comment:5 huhabla]:
I have attached a patch for lib/gis/parser_help.c and lib/gis/parser_stanndard_options.c which implements the discussed ideas.
The patch makes unnecessary formatting changes, which also contravene the GRASS formatting conventions. In general, formatting changes should be kept separate from other changes to make it easier to review the substance of the changes. But in this case, the formatting changes just shouldn't be there.
The updated output is unnecessarily verbose, which is a bad thing IMHO. The "required" and "multiple" status can already be determined from the "Usage:" section. Non-required options are listed in square brackets, while multiple options use the "option=value,..." convention. If you really want this format, it should be a separate option, e.g. --verbose-help.
Probably we should think about the optimalization of the attributes in struct Option (1) Currently we have:
struct Option
{
const char *key;
int type;
int required;
int multiple;
const char *options;
const char **opts;
const char *key_desc;
const char *label;
const char *description;
const char *descriptions;
const char **descs;
char *answer;
const char *def;
char **answers;
struct Option *next_opt;
const char *gisprompt;
const char *guisection;
const char *guidependency;
int (*checker) ();
int count;
};
Parsed for wxGUI, e.g.
{'gisprompt': True, 'multiple': 'no', 'description': 'Name for output vector map', 'guidependency': '', 'default': '', 'age': 'new', 'required': 'yes', 'value': 'lakes_buff', 'label': '', 'guisection': u'Required', 'key_desc': ['name'], 'values': [], 'values_desc': [], 'prompt': 'vector', 'wxId': [-327], 'element': 'vector', 'type': 'string', 'name': 'output'}
I would suggest:
(1) http://download.osgeo.org/grass/grass7_progman/gislib.html#Complete_Structure_Members_Table
Modified by @landam on 13 Apr 2010 13:56 UTC
Comment by glynn on 13 Apr 2010 15:09 UTC Replying to [comment:7 martinl]:
Probably we should think about the optimalization of the attributes in struct Option
Yes please.
I've been suggesting that the "type system" should be re-worked since roughly forever. I would have hoped that the wxGUI development would produce some feedback, but so far that hasn't happened.
Comment by @landam on 13 Apr 2010 15:25 UTC Replying to [comment:9 glynn]:
Replying to [comment:7 martinl]:
Probably we should think about the optimalization of the attributes in struct Option
Yes please.
I've been suggesting that the "type system" should be re-worked since roughly forever. I would have hoped that the wxGUI development would produce some feedback, but so far that hasn't happened.
Can you elaborate a bit on your suggestion? From wxGUI POV it's required to have info about
multiple widget properties
description and label widget label + tooltip
guidependency to updated related widgets (e.g. database -> table -> column)
age widget properties (e.g. show only maps from current maps for 'new')
required options for 'required tab'
value widget value
guisection tabs
prompt for widget type (gselect.*)
type input validation
see also #9
Comment by huhabla on 13 Apr 2010 23:08 UTC Replying to [comment:9 glynn]:
Replying to [comment:7 martinl]:
Probably we should think about the optimalization of the attributes in struct Option
Yes please.
I've been suggesting that the "type system" should be re-worked since roughly forever. I would have hoped that the wxGUI development would produce some feedback, but so far that hasn't happened.
Reworking the "type system" is a good idea. I would be happy to help. My main focus is that most of the data processing grass modules supports the generation of a fully machine readable description of its inputs and outputs (raster, vector, file ... as well as literal data). This will allow the automated generic connection of modules in graphical work-flow builder as well as in WPS process builder. The current approach leads in the right direction but could e improved. E.g.: putting the age, element and prompt into the gisprompt string may be replaced using functions like G_option_set_age(), G_option_set_element() and so on. So the Option structure can be enhanced with additional variables, which are accessed with library functions, so better parameter storage schemes may be implemented and data handling is easier (no parsing overhead)?
From the WPS describe process point of view, i need the following variables:
key Identifier in WPS execute request
multiple maxOccurs in WPS execute request
description and label abstract and title for inputs and outputs
age Is used to distinguish between old -> input (ComplexData, LiteralData) and new -> output (ComplexOutput, LiteralOutput)
required minOccurs in WPS execute request
answer Default value
options Allowed values
prompt Is used to distinguish between raster, vector and file
type LiteralData type
Additionally the following parameter would be very useful:
Comment by @landam on 14 Apr 2010 18:46 UTC Replying to [comment:4 huhabla]:
Martin Landa currently renamed in several modules the input and output parameter to make clear which of them are inputs or outputs. I.e: elevation -> elevation_input, direction -> direction_output, but this is IMHO not a good idea. It is redundant information, especially if you are using the gui.
Right, see [wiki:Grass7/NewFeatures#Renamedoptions Rename options]. Your idea about 'status' info seems to be better solution. If you prefer I can remove '_input/optput' from paramaters key string.
Well this is only my humble opinion. In case other developers prefer the your naming schema i will accept it. Thats why i would like to discuss this topic.
I would personally prefer to remove '_input/optput' from paramaters key string and establish a combination of
key_desc
and the additional "status" parameter in the command line. Maybe we can use a better name than "status" ... like "type"? Additionally we can hide the status parameter in case "input", "map" and "output" are used askey
strings?
OK, done in https://trac.osgeo.org/grass/changeset/41865, [wiki:Grass7/NewFeatures#Renamedoptions Rename options] updated.
Comment by glynn on 15 Apr 2010 12:23 UTC Replying to [comment:10 martinl]:
I've been suggesting that the "type system" should be re-worked since roughly forever. I would have hoped that the wxGUI development would produce some feedback, but so far that hasn't happened.
Can you elaborate a bit on your suggestion?
Currently, the "type" is defined by a combination of fields:
type, required, multiple, options, key_desc, gisprompt.
The type field defines a base type: string, integer or floating-point. gis_prompt may refine the base type to a more specific type. options replaces the base type with an enumeration type. required==NO converts the type to a "Maybe" type. multiple=YES converts the type to a list type (except that the GUI treats it as a set if the "options" field is set). key_desc converts the type to a tuple type if the description contains commas.
My preference would be for a structured type system as is commonly found in high-level languages, where base types can be combined using constructors.
Essentially, all of the above fields would be replaced by a single type field, whose value would be a pointer to an arbitrarily complex structure, with a set of functions for creating types. Off the top of my head, you would need:
// Base types:
T_integer()
T_integer_subrange(int first, int last, int step)
T_integer_enumeration(int count, int val0, [, int val1, ...])
T_real()
T_real_subrange(double first, bool first_inclusive, double last, bool last_inclusive)
T_string()
T_string_enumeration(count, char* val0, [, char* val1, ...])
T_string_enumeration_with_descs(count, char* val0, char *desc0, [, char* val1, char* desc1, ...])
T_element(enum Element element, enum Age age)
// Composite types:
T_optional(Type* base)
T_tuple(int count, Type* type0 [, Type* type1, ...])
T_record(int count, char* name0, Type* type0 [, char* name1, Type* type1, ...])
T_list(Type* type, bool allow_empty, bool allow_duplicates, bool order_matters)
T_union(int count, Type* type0, Type* type1 [, Type* type2, ...])
So e.g.:
opt->type = T_union(2,
T_integer_subrange(1, 99, 2),
T_string_enumeration(2, "all", "none"));
would allow the option value to be any odd integer between 1 and 99 inclusive or the strings "all" or "none".
Variadic functions could either use a prefixed count or a NULL terminator (but the latter won't work for numbers).
You would also need dependent base types, e.g. "column name within the database table associated with the map specified by the input= option".
When it comes to parsing, you could implement such types using a generic mechanism using a callback function to enumerate or validate allowed values; however, we need to think about how this interacts with the GUI.
One possibility would be:
T_dependent(Type (callback)(int argc, char*argv), char opt0 [, char* opt1, ...])
The parser would provide an option to expand an option's type given a partial list of option values, e.g.
d.vect --expand=rgb_column map=inmap
would convert the rgb_column= option's dependent type to a fixed enumeration of the available columns for the specified input map.
The alternative is to simply add each case as a separate base type, i.e.:
T_database_column(char* driver_opt, char* database_opt, char* table_opt)
T_map_database_column(char* map_opt)
The --interface description would simply report the information verbatim and the GUI would be responsible for enumeration.
Comment by huhabla on 17 Apr 2010 22:43 UTC Replying to [comment:13 glynn]:
Replying to [comment:10 martinl]:
[snip]
So e.g.:
opt->type = T_union(2, T_integer_subrange(1, 99, 2), T_string_enumeration(2, "all", "none"));
would allow the option value to be any odd integer between 1 and 99 inclusive or the strings "all" or "none".
That sounds brilliant! I have implemented a simple prototype, to get an idea how this may work. Files are attached. Do you think my implementation points in the right direction?
Variadic functions could either use a prefixed count or a NULL terminator (but the latter won't work for numbers).
You would also need dependent base types, e.g. "column name within the database table associated with the map specified by the input= option".
I am not sure if i understand that, What is the benefit of dependent base type?
When it comes to parsing, you could implement such types using a generic mechanism using a callback function to enumerate or validate allowed values; however, we need to think about how this interacts with the GUI.
The implementation i attached uses a simple callback to generically prints the content of the type structures. The callback will be attached while memory allocation.
struct T_type {
enum T_TYPES type;
void *p; /*Pointer to a type structure*/
void (*print)(struct T_type*);
};
struct Option {
struct T_type *key;
struct T_type *type;
struct T_type *description;
struct T_type *answer;
};
struct Option* T_define_option()
{
struct Option *opt;
opt = (struct Option*)calloc(1, sizeof(struct Option));
opt->key = NULL;
opt->type = NULL;
opt->description = NULL;
opt->answer = NULL;
return opt;
}
struct T_type* T_real_subrange(double first, enum T_BOOL first_inclusive, double last, enum T_BOOL last_inclusive)
{
struct T_type *t;
struct T_type_real_subrange *it;
t = T_type_alloc();
it = (struct T_type_real_subrange*)calloc(1, sizeof(struct T_type_real_subrange));
it->first = first;
it->last = last;
it->first_inclusive = first_inclusive;
it->last_inclusive = last_inclusive;
t->type = REAL_SUBRANGE;
t->p = it;
t->print = T_real_subrange_print;
return t;
}
void T_real_subrange_print(struct T_type* type)
{
struct T_type_real_subrange* p;
p = (struct T_type_real_subrange*)type->p;
if(p) {
fprintf(stderr, "first: %f\n", p->first);
fprintf(stderr, "last: %f\n", p->last);
fprintf(stderr, "first_include: %i\n", p->first_inclusive);
fprintf(stderr, "last_include: %i\n", p->last_inclusive);
}
return;
}
struct Option *opt5 = T_define_option();
opt5->key = T_string("val");
opt5->type = T_real_subrange(100.0, True, 200.0, True);
opt5->description = T_string("Values in between");
//Print the content to stdout
opt5->key->print(opt5->key);
opt5->description->print(opt5->description);
opt5->type->print(opt5->type);
//This will bw the result at stdout:
val
Values in between
first: 100.000000
last: 200.000000
first_include: 1
last_include: 1
One possibility would be:
T_dependent(Type (callback)(int argc, char*argv), char opt0 [, char* opt1, ...])
The parser would provide an option to expand an option's type given a partial list of option values, e.g.
d.vect --expand=rgb_column map=inmap
would convert the rgb_column= option's dependent type to a fixed enumeration of the available columns for the specified input map.
Well, sorry, you lost me at this point. But i think i will catch up, if you could add more examples. :)
I think your approach has enormous potential, but i am also concerned that it may be to complicated for grass module programmers?
Comment by glynn on 18 Apr 2010 14:04 UTC Replying to [comment:14 huhabla]:
I have implemented a simple prototype, to get an idea how this may work. Files are attached. Do you think my implementation points in the right direction?
I think that it's too early to be bothering with implementation details right now. I'd rather clarify the concept then the interface.
Variadic functions could either use a prefixed count or a NULL terminator (but the latter won't work for numbers).
You would also need dependent base types, e.g. "column name within the database table associated with the map specified by the input= option".
I am not sure if i understand that, What is the benefit of dependent base type?
I'm talking about the situation where the set of valid values for one option is dependent upon the value of another option. E.g. for options which specify the name of a database column, valid values are column names in a particular table, typically the table associated with a specific map.
E.g. for d.vect, there are rgb_column=, wcolumn=, size_column=, rot_column=, and attrcolumn= options; valid values for these options are column names from the table associated with the map specified by the map= option. Ideally, the GUI would be able to offer a list of column names once the user has chosen a map.
IIRC, at present the GUI updates the list whenever the user chooses a map. If a module has more than one option whose value is a vector map name, the GUI has no way of knowing which one determines the set of valid column names.
Similar issues exist if valid option values are categories within a raster map, or cell values within the map's range, or coordinates within the map's bounds, subgroups within an imagery group, etc.
When it comes to parsing, you could implement such types using a generic mechanism using a callback function to enumerate or validate allowed values; however, we need to think about how this interacts with the GUI.
Right. This is why I'm inclined towards a "closed sum" approach, with a finite, fixed set of base types and constructors. Having the GUI invoke the module to execute a call-back should be a last resort. Apart from efficiency issues, it forces the GUI to use a generic interface rather than more specialised interfaces.
I think your approach has enormous potential, but i am also concerned that it may be to complicated for grass module programmers?
Most modules will only need relatively straightforward types, e.g. "raster map name". The most common cases will just use G_define_standard_option(). Still-common but more complex cases (e.g. a database column for a particular map) can have utility functions written for them.
Modified by @landam on 12 May 2016 06:44 UTC
Comment by @landam on 23 Aug 2016 12:06 UTC
Comment by @landam on 27 Aug 2016 13:42 UTC Milestone renamed
Comment by neteler on 26 Jan 2018 11:40 UTC Ticket retargeted after milestone closed
Modified by neteler on 12 Jun 2018 20:48 UTC
Comment by @landam on 25 Sep 2018 16:53 UTC All enhancement tickets should be assigned to 7.6 milestone.
Comment by @landam on 25 Jan 2019 21:08 UTC Ticket retargeted after milestone closed
Reported by huhabla on 8 Apr 2010 14:23 UTC I would like to suggest the implementation of a more informative command line help for grass modules. Additionally to the current parameters the status(age), type and if it is required or not should be displayed.
A patch for parser_help.c is attached.
Example r.buffer:
Will change into
GRASS GIS version and provenance
svn-trunk
Migrated-From: https://trac.osgeo.org/grass/ticket/1031