-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Commit
Can't set parameters to null (None or '') if the parameter is not optional
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,11 @@ def setValue(self, obj): | |
Returns true if the value passed is correct for the type | ||
of parameter. | ||
""" | ||
if obj is None: | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
self.value = unicode(obj) | ||
return True | ||
|
||
|
@@ -111,6 +116,8 @@ def __init__(self, name='', description='', default=True): | |
|
||
def setValue(self, value): | ||
if value is None: | ||
if not self.optional: | ||
return False | ||
self.value = self.default | ||
return True | ||
if isinstance(value, basestring): | ||
|
@@ -133,6 +140,8 @@ def __init__(self, name='', description='', default='EPSG:4326'): | |
|
||
def setValue(self, value): | ||
if value is None: | ||
if not self.optional: | ||
return False | ||
self.value = self.default | ||
return True | ||
|
||
|
@@ -167,7 +176,9 @@ def __init__(self, name='', description='', default='0,1,0,1'): | |
|
||
def setValue(self, text): | ||
if text is None: | ||
self.value = self.default | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
m-kuhn
Member
|
||
return True | ||
tokens = text.split(',') | ||
if len(tokens) != 4: | ||
|
@@ -203,8 +214,7 @@ def setValue(self, obj): | |
if self.value.strip() == '' or self.value is None: | ||
if not self.optional: | ||
return False | ||
else: | ||
self.value = '' | ||
self.value = '' | ||
if self.ext is not None and self.value != '': | ||
return self.value.endswith(self.ext) | ||
return True | ||
|
@@ -229,6 +239,11 @@ def __init__(self, name='', description='', numRows=3, | |
self.value = None | ||
|
||
def setValue(self, obj): | ||
if obj is None: | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
# TODO: check that it contains a correct number of elements | ||
if isinstance(obj, (str, unicode)): | ||
self.value = obj | ||
|
@@ -276,11 +291,10 @@ def __init__(self, name='', description='', datatype=-1, optional=False): | |
def setValue(self, obj): | ||
self.exported = None | ||
if obj is None: | ||
if self.optional: | ||
self.value = None | ||
return True | ||
else: | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
|
||
if isinstance(obj, list): | ||
if len(obj) == 0: | ||
|
@@ -417,7 +431,9 @@ def __init__(self, name='', description='', minValue=None, maxValue=None, | |
|
||
def setValue(self, n): | ||
if n is None: | ||
self.value = self.default | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
try: | ||
if float(n) - int(float(n)) == 0: | ||
|
@@ -453,7 +469,9 @@ def __init__(self, name='', description='', default='0,1'): | |
|
||
def setValue(self, text): | ||
if text is None: | ||
self.value = self.default | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
tokens = text.split(',') | ||
if len(tokens) != 2: | ||
|
@@ -510,11 +528,10 @@ def getSafeExportedLayer(self): | |
def setValue(self, obj): | ||
self.exported = None | ||
if obj is None: | ||
if self.optional: | ||
self.value = None | ||
return True | ||
else: | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
if isinstance(obj, QgsRasterLayer): | ||
self.value = unicode(obj.dataProvider().dataSourceUri()) | ||
return True | ||
|
@@ -552,6 +569,8 @@ def __init__(self, name='', description='', options=[], default=0, isSource=Fals | |
|
||
def setValue(self, n): | ||
if n is None: | ||
if not self.optional: | ||
return False | ||
self.value = self.default | ||
return True | ||
try: | ||
|
@@ -577,10 +596,9 @@ def __init__(self, name='', description='', default='', multiline=False, | |
|
||
def setValue(self, obj): | ||
if obj is None: | ||
if self.optional: | ||
self.value = '' | ||
return True | ||
self.value = self.default | ||
if not self.optional: | ||
return false | ||
self.value = '' | ||
return True | ||
self.value = unicode(obj).replace( | ||
ParameterString.ESCAPED_NEWLINE, | ||
|
@@ -604,11 +622,10 @@ def __init__(self, name='', description='', optional=False): | |
def setValue(self, obj): | ||
self.exported = None | ||
if obj is None: | ||
if self.optional: | ||
self.value = None | ||
return True | ||
else: | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
if isinstance(obj, QgsVectorLayer): | ||
source = unicode(obj.source()) | ||
self.value = source | ||
|
@@ -679,7 +696,10 @@ def getValueAsCommandLineParameter(self): | |
|
||
def setValue(self, value): | ||
if value is None: | ||
return self.optional | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
elif len(value) > 0: | ||
self.value = unicode(value) | ||
else: | ||
|
@@ -721,11 +741,10 @@ def __init__(self, name='', description='', shapetype=[-1], | |
def setValue(self, obj): | ||
self.exported = None | ||
if obj is None: | ||
if self.optional: | ||
self.value = None | ||
return True | ||
else: | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
if isinstance(obj, QgsVectorLayer): | ||
self.value = unicode(obj.source()) | ||
return True | ||
|
@@ -814,7 +833,10 @@ def getValueAsCommandLineParameter(self): | |
|
||
def setValue(self, value): | ||
if value is None: | ||
return self.optional | ||
if not self.optional: | ||
return False | ||
self.value = None | ||
return True | ||
elif len(value) == 0: | ||
return self.optional | ||
if isinstance(value, unicode): | ||
|
29 comments
on commit 3472ac8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional does not remove de need for a defualt value. The default is shown to the user in the UI, so the UI must be able to somehow retrieve that value. I think an optional value is just one that can accept a None valu (or not be present when calling the algorihtm, so it can be omitted), so it's more in terms of syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem i see is that the meaning of "optional" might not be the same for all types of parameters. For instance, an extent can really be "optional", meaning that, the algorithm can run without it. If you have a extent parameter called "Crop result to extent", if you do not add it, it does not crop anythin but the algorithm runs normally. For a number value, like the "buffer distance" in a buffer algorithm, optional means "use whatever default value you have", but still that value is needed. It cannot be None.
All numerical values in the current algorithms are like that. That's the reason why there were no optiona numbers before, but i added them as requested, and the same for other types of parameters. But the meaning of being optional is not exactly the same in all cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you agree that the concept of optional in general is to ask the algorithm to do whatever it thinks is best? This can depend on the algorithm and parameter obviously. Just what happens if an external binary is called without specifying a particular parameter.
Anyway, do you agree that the behavior should be the following?
- If a parameter is optional and None is given: omit it.
- If a parameter is not optional and None is given: use the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If a parameter is not optional and None is given: use the default."
That not the case in parameters such us layers or tables (the only ones that were optional at first). If it's not optional (like a DEM in a Slope algorithm), None is not allowed, there must be a valid layer, otherwise it throws an exception. In this case, there is no default
Other than that, what you propose makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense for other parameters to define None as default value for non-optional parameters?
Or can they all throw exceptions in this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They all should throw exceptions if None is not a valid value...and there is no default value to resort to in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be sure, I'd like to resume the comportement if value is set to None:
- For non-optionnal parameter without default value (layer, table, etc): throw an exception
- For non-optionnal parameter with default value (String, Number, etc): set value to default ? or throw an error ?
- For optionnal parameter with default value: set value to default ? or to None ?
- For optionnal parameter without default value: set value to None
I'm not sure to have well understand what you should when a parameter has a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on improving the test coverage of the parameters module, and am interested in incorporating the results of this discussion.
Just from a code standpoint, I don't get why "optional" parameter is given a default value when the user has assigned it to be None. That's not optional per se, it's just optional for the user to specify it. It will always have a value, it just might be the default specified in the constructor. If you are saying that you want it to be "optional" for the user to specify, but it must always have some Value can that be accomplished by making it required and have a default?
Examples are helpful - Do any algorithms come to mind that use these parameters in this interesting way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we mixing up parameter type and parameter here?
I'm making the following example up, just to show the point but don't blame me on them
- A parameter type CRS can be optional or have a default
- An algorithm reproject does have a parameter (of type CRS) target CRS. This is not optional and can have a default of 4326 (meaning: if not specified otherwise)
- An other algorithm rasterize does have a parameter (of type CRS) target CRS. This is optional as in: if unspecified it will be taken from the source layer.
I don't see why different parameter types should handle optional and default values differently. It should be possible to specify a default for all of them (although it doesn't make much sense for a layer, but why prohibit it?) and to make them all optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I've never written a processing plugin 😊
Aren't we mixing up parameter type and parameter here?
I'm not sure what you mean. Are you trying to distinguish between processing.core.parameters.Parameter()
and processing.core.parameters.Parameter().value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And neither did I 😊
I am trying to distinguish between the class processing.core.parameters.Parameter
(as in paramter type) and an instance of it (as in parameter).
The class should support the concept of being optional while each instance can be tagged if it is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I'd like to know if we have to update the way Processing manage optional parameter.
I'd like to know how to manage set value :
- For non-optionnal parameter with default value (String, Number, etc): set value to default ? or throw an error ?
- For optionnal parameter with default value: set value to default ? or to None ?
Second question, for which parameter type do you think Processing not well manage set value to None ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I do not feel authoritative for this at all. I'd really appreciate @volaya to take the decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made explicit some of the optional/default behavior in #2541
See the various testOptional methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I met @rldhont last week at the Frech QGIs meeting adn we discussed about this. He will write down the cases and how parameters should behave, according to that discussion, and put it here so you can check it, and I will implement that.
The rationale is that the design of parameters was mainly conceived for using Processing from QGIS desktop (toolbox, console, etc), but for the case of using it in the server (WPS), the situation is a bit different,, as he is finding out. Changes are not likely to affect the desktop interface or break stuff, but will allow him to have a better implementation on the server side. Hopefully, they will not look counterintuitive on the desktop side of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @volaya @michaelkirk and @m-kuhn
Here is the way, I think, the setValue method has to be:
def setValue(self, obj):
if obj is None:
self.value = self.default
if not self.optional and self.default is None:
return False
return True
self.value = unicode(obj)
return True
I think that if the parameter's value is set to None the value has to be the default one and this set is valid (return True) if the parameter is optional or if the parameter is not optional but with a not None default value.
This can be applied to all parameters:
Param | not optional and default is None | not optional and default not None | optional and default not None | optional and default is None |
---|---|---|---|---|
Boolean | x | set to default and return True | set to default and return True | x |
Crs | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Extent | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
File | set to '' and return False | set to default and return True | set to default and return True | set to '' and return True |
FixedTable | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
MultipleInput | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Number | set to default and return False | set to default and return True | set to default and return True | set to default and return True |
Range | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Raster | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Extent | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Selection | x | set to default and return True | set to default and return True | x |
String | set to '' and return False | set to default and return True | set to default and return True | set to '' and return True |
Table | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
TableField | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Vector | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
GeometryPredicate | set to None and return False | set to default and return True | set to default and return True | set to None and return True |
Are-you agree with all of these ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for the comments of @m-kuhn and @michaelkirk, but that sounds good to me. Pity that we cannot use that code snippet for the parent Parameter class (since there are specific checks for each parameter type), but it should be an easy work to adapt the parameters module to this. I will work on it if no one says anything against it
Thanks for the nice summary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started working on this. Have a minor comment.
What happens if the default is a not-None value (let's say, for a crs parameter, the default is "EPSG:4326") but the user wants to set None as the value (for instance, to indicate that there is no need to use any CRS for reprojecting the output)?
I think that this is a corner case, and probably be handled with a subclass of the ParameterXXX class, but just wanted to mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice list @rldhont . I'd say this should go into the documentation somewhere so this work is not in vain.
Concerning the snippet: If False
is returned, I would suggest the value
should not be changed at all.
@volaya Can we use this in the parent class and implement a setValueCheck
method only where required in subclasses which is called from the default setValue
method?
Or overwrite setValue
it in subclasses to do the check and then call the parent's default implementation?
@volaya I think the None
value is used ambiguously: For "not set" and for "set default". There should probably be a value Parameter.default
or similar that can be used to explicitly specify using the default value.
None
should be reserved for "not set". For mandatory (non-optional) parameters None
can be treated as Parameter.Default
or even better (if it doesn't break existing code) an exception could be thrown : "You are trying to set None
on a mandatory parameter: bad idea!".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch @m-kuhn. All parameters shoud have a parameter.default value, so instead of using None, one could do param.setValue(param.default), and leave setValue(None) to indicate that the parameter doesnt have a value and should not be used (which would return false if the parameter is mandatory).
That was more or less how it was before, i think... I would let @rldhont comment on this approach. I find it easier to understand, but not sure if it might cause some issues in the QGIS server side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @volaya and @m-kuhn for your review.
Firstly, what about set value to the default one during instantiation ?
@volaya for crs parameter, the default value can be None, because if the parameter is optional he can not be set.
@m-kuhn, I think that do a setValue to None is equal to reset value to default. And setValue returns if the setValue done is valid for running the algorithm.
For me the problem with not modifying value when None is passed, is that the parameter will stay at the previous value. It will never arrive in the UI, but can be do in python script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm repeating what @m-kuhn described, but just to reiterate maybe:
param.setValue(arg)
should do one of two things -
1.) When the arg is valid*, set the param.value, return true (this may entail mutating the arg, e.g. extent parameter )
2.) When the arg is invalid, don't touch param.value, return false
*Validity is described by the ParameterType (e.g. a NumberParameter should get a number value) and optionality (setting None to a required type is invalid).
The way to reset the default should be param.setValue(param.default) or we could add a param.setDefaultValue if we want to get fancy. Equating param.setValue(None)
to param.resetDefaultValue
is not intuitive.
Thanks for putting this extensive table together @rldhont. I think executable test cases are a good form of documentation for something like this. I've spelled out some of the test cases in #2541, consider using it as part of your changes @volaya.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for bundling this with #2541
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to make it clear, the proposal in the last comment by @michaelkirk ...would that be ok for you, @rldhont ? I find it to be intuitive and nice
If in the server side you need to set the default if None is passed, you can put that logic outside of Processing itself, in the server code, and if the value is None, call param.setDefaultValue()
Another thing we can do is to expose a method param.checkValidity(value), so you can check the validity of a value for a given parameter, without changing its current value
That might add some extra code to the server side, but it will leave the implementation in Processing much clearer and more intuitive.
@m-kuhn, @michaelkirk: opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For server side, I use Processing.runAlgorithm(alg, None, args) and I need that some input has None value. So If we add checkValidity or setDefaultValue, I'll update this method.
@volaya, @m-kuhn and @michaelkirk: how we organize us ?
First merge #2541 ? Then add CheckValidity and setDefaultValue ? At last update rumAlgorithm ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say start with setDefaultValue()
and runAlgorithm()
in this PR, then merge #2541 into this PR and make sure the two are compatible. Not sure about checkValidity()
afaics it's an optional added value which can easily be added later?
Thank you all for your work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To continue the discussion, you'll find proposal code at the PR #2590 [Processing] Manage default value for parameter
Should that return self.default instead of None? If not, what is the member variable self.default used for? Starting to work on a test suite and some old tests complain here and I wonder if I need to adjust the test or the code.