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 bylocation operators #1759

Merged
merged 1 commit into from
Feb 8, 2015

Conversation

arnaud-morvan
Copy link
Contributor

Adds a new ParameterGeometryPredicate class and related widget GeometryPredicateSelectionPanel.

With this new widget, the user can select precisely the predicates he wants in this list:
('intersects', 'contains', 'disjoint', 'equals', 'touches', 'overlaps', 'within', 'crosses')
in an boolean "or" like manner.

Alter SelectByLocation, ExtractByLocation and SpatialJoin (Join attributes by location) algorithms to use this new parameter type.

The first input layer is used as left operand and the second as the right operand, this seems to natural in user point of view.

Maybe this can break some scripts using altered algorithms.

After merge, I'll make a pull request to add he new ParameterGeometryPredicate in processing console mode documentation.

In modeler

Fully fonctionnal.

In batch mode

Display all checkboxes horizontally.

In console mode

import processing
processing.runalg("qgis:selectbylocation",
                  "ARRONDISSEMENT",
                  "DEPARTEMENT",
                  ["within"],
                  0)

@gioman
Copy link
Contributor

gioman commented Jan 9, 2015

assigned @volaya @alexbruy

@@ -770,3 +770,41 @@ def dataType(self):
types += 'any, '

return types[:-2]


class ParameterPredicate(Parameter):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it will be better to use ParameterGeometryPredicate as we also might want to add other predicates in future, e.g. boolean or SQL

@alexbruy
Copy link
Contributor

Thanks for update! Am I right that in batch mode one should enter predicates by hand as comma-delimited string? It is not very user friendly and also don't allow us to have localized predicates.
All other looks good for me.

@volaya can you look at this too?

@arnaud-morvan
Copy link
Contributor Author

Rebased on master

@arnaud-morvan
Copy link
Contributor Author

I've enabled the specific predicate widget in batch dialog

@arnaud-morvan
Copy link
Contributor Author

@alexbruy @volaya
Works well with modeler and batch dialog.
Maybe this could be merge now.
I am not completely aware of parameter method getValueAsCommandLineParameter, in which context is it used ?

@volaya
Copy link
Contributor

volaya commented Jan 21, 2015

Looks good to me, but in the batch processing interface i just see a set of checkboxes with no captions, so it's impossible to use it. Fixing that, it should be ready to merge.

Works fine in modeler, but it loses the ability to expose the parameter to the model user. That is, before you could set a boolean parameter and pass that value to the algorithm parameters. Now you don't have an option to do that.

A better option in this case, IMHO, would be to leave the original boolean parameters and then create a custom UI using your widget (which is great, btw). That would result in clearer semantics (I would prefer not to add more parameter types, specially if they are going to be used by only a few algorithms), and would not break previous scripts using the algorithm.

I might consider that myself as a future work (if you agree), but for now, if the batch stuff gets fixed, i will merge this.

Great work!

@arnaud-morvan
Copy link
Contributor Author

I've fixed the batch dialog column width problem, specially for this parameter type.

@arnaud-morvan arnaud-morvan force-pushed the processing_bylocation_operators branch 2 times, most recently from d8880da to c5e7cc2 Compare January 23, 2015 08:48
@volaya
Copy link
Contributor

volaya commented Feb 5, 2015

@arnaud-morvan Sorry for the late reply. We were already feature freeze, so I alex and I were reluctante to merge, but I discussed with Jurgen and it seems it is fine, since these PR (and your other one are safe). However, I am seeing that there are conflicts now. Could you fix that and update the PR? Once it is done, I will inmediately merge them. Sorry for the delay

@arnaud-morvan
Copy link
Contributor Author

Thanks, I've rebased on master and added some self.tr() for translation consistency.

I've also interverted the geometric predicate operands in selectbylocation and extractbylocation, so the first layer parameter is the left operand, and the second layer parameter is the right operand. This seems to be more natural for the user, and is consistent with SpatialJoin previous behavior. (this consideration only affect non symetric operators : contains and within)

I've tested batch, modeler and console (in console, there is an error due to recent commit from Jurgen in context of a pep8 check, but it works good by importing runalg from processing.tools.general module).

Regards

volaya added a commit that referenced this pull request Feb 8, 2015
@volaya volaya merged commit afd21cd into qgis:master Feb 8, 2015
@arnaud-morvan arnaud-morvan deleted the processing_bylocation_operators branch August 31, 2015 14:16
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

5 participants