Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Processing] Optional parameter groups #120

Open
nyalldawson opened this issue Apr 10, 2018 · 18 comments
Open

[Processing] Optional parameter groups #120

nyalldawson opened this issue Apr 10, 2018 · 18 comments

Comments

@nyalldawson
Copy link
Contributor

nyalldawson commented Apr 10, 2018

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

  • QgsProcessingAlgorithm, associated Processing unit tests
  • Grass7Algorithm.py
  • description files for affected GRASS algorithms

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
Copy link

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
Copy link

gioman commented Apr 10, 2018

@nyalldawson thanks (a lot) for tackling this.

@luipir
Copy link

luipir commented Apr 10, 2018

+1

@pcav
Copy link
Member

pcav commented Apr 10, 2018

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

@nyalldawson
Copy link
Contributor Author

@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
Copy link
Contributor Author

@cbertelli

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

@pcav
Copy link
Member

pcav commented Apr 10, 2018

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

@cbertelli
Copy link

cbertelli commented Apr 10, 2018 via email

@nyalldawson
Copy link
Contributor Author

@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
Copy link

rouault commented Apr 10, 2018

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
Copy link
Member

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
Copy link

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

@nyalldawson
Copy link
Contributor Author

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

@nyalldawson
Copy link
Contributor Author

Implemented in qgis/QGIS#6819

@alexbruy
Copy link
Contributor

@nyalldawson do you plan to revive this?

@nyalldawson
Copy link
Contributor Author

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

@wonder-sk
Copy link
Member

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
Copy link
Contributor Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants