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] Add method for algorithms to preprocess parameter values #6658

Merged
merged 1 commit into from
Apr 3, 2018

Conversation

nyalldawson
Copy link
Collaborator

Allows algorithms to pre-processes a set of parameters, allowing the algorithm to clean their values.

This method is automatically called after users enter parameters, e.g. via the algorithm dialog. This method should NOT be called manually by algorithms.

Allows algorithms to pre-processes a set of parameters, allowing the
algorithm to clean their values.

This method is automatically called after users enter parameters, e.g.
via the algorithm dialog. This method should NOT be called manually
by algorithms.
@nyalldawson
Copy link
Collaborator Author

@alexbruy how's this look? Relates to #6272

@m-kuhn
Copy link
Member

m-kuhn commented Mar 22, 2018

Pretty similar to what I am planning to do for the filter algorithm. But I think we will there not only require to return an updated variant map, but actually analyze it again for outputs etc.
Something like:

if alg.preprocessParameters(parameters):
    # True -> something changed, analyze parameters again
    parameters = alg.updatedParameters()
    self.analyze_parameters(parameters) # Analyze them again (create outputs etc...)

https://github.com/nyalldawson/QGIS/blob/402a6f789c1e8bb52d2577aaf6f3a66696764d25/python/plugins/processing/gui/AlgorithmDialog.py#L94-L119

I am still not completely sure about the best way to do it...

@nyalldawson
Copy link
Collaborator Author

@m-kuhn I'm not sure that's the same use case. Can you give some more details (or link to code)?

@rkanavath
Copy link
Contributor

@m-kuhn , its in the AlgorithmDialog getParameterValues method. So analyze can also be done at end of preprocessParameters()
you can see one usage here: https://gitlab.orfeo-toolbox.org/orfeotoolbox/qgis-otb-plugin/blob/master/otb/OTBAlgorithm.py#L187

If you add a preprocessParameters in algorithm class, it could take care of situation where you have removing/adding output parameters and do the analyze.
Does that work for you?

@m-kuhn
Copy link
Member

m-kuhn commented Mar 23, 2018

Also not sure about the use case, but it potentially affects the same API.

What needs to be done is that outputs are created based on configuration, so I guess it's a bit more complicated in the requirements than the one here.

What I was thinking about is a return value that gives instructions to the caller if the algorithm characteristics (parameters) have changed and need to be checked again. I think this would require a boolean return value instead of a modified map.

Something like this, but there would be quite some variations possible as well:

bool preprocessParameters( QVariantMap );
// or bool setParameters( QVariantMap );

QgsProcessingParameterDefinitions parameterDefinitions(); // returns also "generated" parameters

QVariantMap parameters(); // Returns original parameters configuration + generated parameter configuration - cleaned parameter configuration

@nyalldawson
Copy link
Collaborator Author

@m-kuhn

I'd like to merge this if you're ok with it. It addresses an urgent current use case and is easily backportable. We could then move discussion about your requirements to a different PR when there's code available to visualise how you need things to work...

@m-kuhn
Copy link
Member

m-kuhn commented Apr 3, 2018

@nyalldawson ok to backport as a hotfix. I'd be glad if possibly if you could give me a short feedback on the above if possible (as in: "no-go", "yes, but you could try ... instead", "yes please"), I'll be happy to continue with this soon.

@nyalldawson
Copy link
Collaborator Author

@m-kuhn I can't really comment without knowing the use case for it. It does seem a bit odd requiring iterative calls, but without seeing how you'd use it I can't suggest an alternative!

@nyalldawson nyalldawson merged commit 08d30c3 into qgis:master Apr 3, 2018
@nyalldawson
Copy link
Collaborator Author

@rkanavath I think all the PRs you needed are merged now, let me know if there's anything else you need.

@m-kuhn
Copy link
Member

m-kuhn commented Apr 3, 2018

well, backport as a hotfix isn't equal to merge to master...
but well, so be it and we'll probably have two api changes here. I'll contact you off-list to pick your brain if you don't mind.

@rkanavath
Copy link
Contributor

@nyalldawson , All change needed for otb plugin to move on is ready. But,
I will open one more PR that will parse only name and group name for speedup processing. However this is optional and we can go with or without depending on this feature in next release. It will help others too..

@nyalldawson nyalldawson deleted the preprocess branch April 9, 2018 06:38
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

Successfully merging this pull request may close these issues.

None yet

3 participants