qgis / QGIS-Enhancement-Proposals

QEP's (QGIS Enhancement Proposals) are used in the process of creating and discussing new enhancements for QGIS
118 stars 37 forks source link

[Processing] Optional parameter groups #120

Open nyalldawson opened 6 years ago

nyalldawson commented 6 years ago

QGIS Enhancement: [Processing] Optional parameter groups

Date 2018/04/10

Author Nyall Dawson (@nyalldawson)

Contact nyall dot dawson at gmail com

Version QGIS 3.0 (via backports)

Summary

While Processing algorithms currently have support for specifying whether an individual input parameter is optional or not, this support only works for independent optional parameters. There is no way for algorithms to precisely specify how optional parameters operate and no support for groups of optional parameters (e.g. "at least one of these parameters must be set" or "at most one of these parameters must be set").

This support is required for algorithms with dependant optional parameters, e.g. an algorithm which allows users to specify an output raster size via either a x/y resolution OR via number of rows and columns. GRASS algorithms in particular require heavy use of dependent optional parameters.

Some existing algorithms attempt to work around this missing functionality by marking all affected parameters as optional, but that does not prevent users from skipping all the parameters OR entering values for a set of mutually exclusive parameters. Users must then interpret any errors thrown on the processing log to determine whether they have skipped a set of required parameters or entered values for two mutually-exclusive parameters. (This is especially difficult for the GRASS provider algorithms, where the log is very noisy and it is difficult to determine from reading the log exactly what caused the grass command execution to fail).

This proposal covers extending the QgsProcessingAlgorithm API to allow algorithms to specify groupings for their optional parameters, including mutually exclusive groups and "at least one parameter" type groups.

Proposed Solution

QgsProcessingAlgorithm API Changes

A new enum will be added to QgsProcessingAlgorithm:

/**
 * Parameter group types.
 * \since QGIS 3.0.2
*/
enum ParameterGroupType
{
    AtLeastOneRequired, //!< At least one of the grouped parameters must be specified, and more than one may be specified
    MutuallyExclusiveOptional, //!< At most one of the grouped parameters must be specified
    MutuallyExclusiveRequired, //!< One parameter from the group MUST be specified
};

A new struct for parameter groups will also be added to QgsProcessingAlgorithm:

/**
 * Contains settings related to a single group of linked parameters.
 * \since QGIS 3.0.2
*/
struct ParameterGroup
{
   //! Names of parameters contained within the group
   QSet< QString > parameters;

   //! Type of group
   ParameterGroupType type = AtLeastOneRequired;
}

New methods will be added to allow algorithms to set and retrieve parameter groups:

/**
 * Adds a new parameter \a group to the algorithm. Parameter groups specify how sets of related optional parameters are handled.
 * 
 * Parameter groups are stored by unique ParameterGroup::parameters sets. Adding a new parameter group with the
 * same set of parameters as an existing group will cause the existing group to be removed and replaced
 * with the new group.
 *
 * \since QGIS 3.0.2
 * \see parameterGroups()
 */
addParameterGroup( const ParameterGroup& group );

/**
 * Returns the list of parameter groups used by the algorithm.
 *
 * \since QGIS 3.0.2
 * \see addParameterGroup()
 */
QList< ParameterGroup > parameterGroups() const;

QgsProcessingAlgorithm::checkParameterValues will be modified to add an iteration through the algorithm's parameter groups, applying the check corresponding to the group's ParameterGroupType. Accordingly parameter maps which do not pass the constraints specified by the algorithm's parameter groups will cause the checkParameterValues test to fail, showing a friendly error message in the algorithm dialog.

Processing GRASS provider changes

The Grass7Algorithm defineCharacteristicsFromFile method will be modified to check for the presence of lines starting with a #group= string. This line will be then treated as a pipe delimited list of group type followed by parameter names. E.g. for the r.cost start group, the description file would contain the line

#group=MutuallyExclusiveRequired:start_coordinates|start_points|start_raster

This indicates that either the optional start_coordinates, start_points, or start_raster parameters must be specified, but only ONE of these parameters can be specified at once.

Affected Files

Performance Implications

N/A

Further Considerations/Improvements

N/A

Backwards Compatibility

Will be backported to 3.0 to allow fixes to GRASS algorithms in QGIS 3.0

Votes

(required)

cbertelli commented 6 years ago

It's a very good thing for GRASS, but also for GDAL which was born as an incremental collection of tools and, despite the efforts of Even @rouault, is somehow less coherent. Bridging command line and GUIs has a long story, is there anything that can provide a more general solution to the shortcomings of the QgsProcessingAlgorithm API? Anyway this is much needed enhancement, whatever the solution proposed.

gioman commented 6 years ago

@nyalldawson thanks (a lot) for tackling this.

luipir commented 6 years ago

+1

pcav commented 6 years ago

In principle I agree. Does this have any overlap with the work on OTB provider by Rashad?

nyalldawson commented 6 years ago

@pcav no overlap! In fact, OTB was another provider I had in mind with this as it should make that provider more user friendly also

nyalldawson commented 6 years ago

@cbertelli

Have you got some examples of mutually exclusive gdal settings? I'll add them to the QEP description.

pcav commented 6 years ago

Thanks @nyalldawson . I kind of remember they also implement something similar. Then +1 for me.

cbertelli commented 6 years ago

@nyalldawson

I'm always not completely sure. I see that every time I tackle my problems with gdal_translate or gdal_warp I need to stop, re-read the manual, and try to apply the options to my specific needs. I may find stupid examples, say in gdal_warp the options -cutline , -cwhere

and -csql . I think Even @rouault has a more precise idea of these mutually exclusive or partially complementary options. Given time, and if it is useful for someone, I may dig in the man pages. c On Tue, Apr 10, 2018 at 9:54 PM, Nyall Dawson wrote: > @cbertelli > > Have you got some examples of mutually exclusive gdal settings? I'll add > them to the QEP description. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > , > or mute the thread > > . > -- -------------------------------------------------------------------------- Carlo A. Bertelli Charta servizi e sistemi per il territorio e la storia ambientale srl Dipendenze del palazzo Doria, vc. alla Chiesa della Maddalena 9/2 16124 Genova (Italy) tel./fax +39(0)10 2475439 +39 0108566195 mobile:+39 393 1590711 e-mail: bertelli@chartasrl.eu http://www.chartasrl.eu --------------------------------------------------------------------------
nyalldawson commented 6 years ago

@cbertelli

No worries - the first step is getting the API in place to handle this anyway, and then we can add to gdal algs on a case by case basis

rouault commented 6 years ago

Not sure which extra feedback you are especting more of me. But yes there are definitely mutually exclusive options in GDAL/OGR utilities (like using ogr2ogr with layernames or with -sql). This should generally be documented (although I'd not be surprised that the documentation might just be errors thrown at runtime in some cases. would be nice to improve the doc if you find such occurences).

wonder-sk commented 6 years ago

In the proposal's summary you give a good example of output raster size specification: a user can either set number of rows and columns (nrows, ncols) or set x/y resolution (xres, yres). How would we capture the requirement that we require both nrows, ncols or we require both xres, yres, but we do not want a mixture (e.g. nrows + xres) ? Don't we need sub-groups to be able to model more complex logic?

With sub-groups we could express the above example as a group with two sub-groups:

mutually_exclusive_required( all_required( nrows, ncols ), all_required( xres, yres ) )

In "Align rasters" algorithm, there is a similar situation - user can either use default cell size or specify x/y resolution - but specifying just xres or just yres is invalid input. This could be recorded as:

mutually_exclusive_optional( all_required( xres, yres ) )

My thinking is that the groups could even form arbitrary logical clauses (e.g. a => b & (c | d) where a, b, c, d are variables stating whether a parameter is given). I can see this used also when an algorithm has multiple modes of operation, each mode requiring a different set of parameters.

But I am probably over-engineering this! :-)

rkanavath commented 6 years ago

@nyalldawson , This QEP can remove use of parameter.metadata() and custom widget wrapper (currently in use) for OTB parameter choice. is that correct?

nyalldawson commented 6 years ago

@wonder-sk good point - i've adapted for that.

nyalldawson commented 6 years ago

Implemented in https://github.com/qgis/QGIS/pull/6819

alexbruy commented 5 years ago

@nyalldawson do you plan to revive this?

nyalldawson commented 5 years ago

@alexbruy not really. Discussion in qgis/QGIS#6819 reached a deadlock and no acceptable approach was found that kept everyone happy.

wonder-sk commented 5 years ago

Oh I didn't mean to block this good effort - if others are generally happy with the original design I withdraw my proposal for different logic of evaluation :)

nyalldawson commented 5 years ago

@wonder-sk I don't think it's like that - it's more a sign that it needs to be totally rethought in order to satisfy everyone's needs