QGIS raster calculator: better handling of optional parameters and error message

[qgib]

Author Name: Giovanni Manghi (@gioman)
Original Redmine Issue: 19553
Affected QGIS version: 3.7(master)
Redmine category:processing/qgis

---

See attachment.

Tested on QGIS master on Linux.

I guess the "reference layer(s)" option should be made mandatory or the error must be caught.

---

• Screenshot_20180806_181109.png (Giovanni Manghi)

[qgib]

Author Name: Alexander Bruy (@alexbruy)

---

This is not error. Raising QgsProcessingExcpetion is a normal Processing way to report errors like this, others algorithms behaves in the same manner.

---

• priority_id was changed from High to Normal
• category_id was changed from Raster Calculator to Processing/QGIS
• status_id was changed from Open to Feedback

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Alexander Bruy wrote:

This is not error. Raising QgsProcessingExcpetion is a normal Processing way to report errors like this, others algorithms behaves in the same manner.

Then I guess that in all such tools the mandatory "flag" is missing in some parameter, correct?

---

• status_id was changed from Feedback to Open

[qgib]

Author Name: Luigi Pirelli (@luipir)

---

should we check if the field have to be mandatory or not... and this depend on the algorithm implementation.
I¡m on holiday and I can start to check next week.

---

• assigned_to_id was configured as Luigi Pirelli

[qgib]

Author Name: Alexander Bruy (@alexbruy)

---

Giovanni Manghi wrote:

Then I guess that in all such tools the mandatory "flag" is missing in some parameter, correct?

No. I just saying that raising QgsException is a correct way to report any errors in Processing.

Speaking about raster calulator algorithm, reference layer is optional parameter and this is correct. Because user can choose how to define output: using reference layer or entering cellsize, extent and crs. There is no issue here at all.

---

• status_id was changed from Open to Feedback

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Alexander Bruy wrote:

Giovanni Manghi wrote:

Then I guess that in all such tools the mandatory "flag" is missing in some parameter, correct?

No. I just saying that raising QgsException is a correct way to report any errors in Processing.

Hi Alex,
ok

Speaking about raster calulator algorithm, reference layer is optional parameter and this is correct. Because user can choose how to define output: using reference layer or entering cellsize, extent and crs. There is no issue here at all.

does not seems the case, without that optional parameter the calculator does not work (and throws the error). There is no way to use the tool without the optional parameter at the moment (which is the reason it is confusing at the moment).

[qgib]

Author Name: Alexander Bruy (@alexbruy)

---

Giovanni Manghi wrote:

does not seems the case, without that optional parameter the calculator does not work (and throws the error). There is no way to use the tool without the optional parameter at the moment (which is the reason it is confusing at the moment).

I can't confirm. As said in my previous comment, if reference layer is not set but all other parameters needed to describe raster (extent, cell size and crs) are set algorithm works as expected. And this is correct, either reference layer should be provided or extent, cell size and crs.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

I can't confirm. As said in my previous comment, if reference layer is not set but all other parameters needed to describe raster (extent, cell size and crs) are set algorithm works as expected. And this is correct, either reference layer should be provided or extent, cell size and crs.

ok Alex, let me rephrase: all parameters are optional (reference layer, CRS, extent and cell size), so from a user point of view a tool should NOT throw an error if none of the optional parameters is defined.

If either the reference layer OR the other 3 parameters are mandatory, and if Processing can't handle this scenario, then the tool should be split in two.

If the tool is to be kept as one, the the above python error should be caught and replaced with something more user friendly (I know, the python error is clear if a user read it, but yet users tend not to read them.

my 2 cents.

---

• subject was changed from QGIS raster calculator: python error if no reference layers is selected to QGIS raster calculator: better handling of optional parameters and error message
• regression was changed from 1 to 0

[qgib]

Author Name: Alexander Bruy (@alexbruy)

---

Giovanni Manghi wrote:

ok Alex, let me rephrase: all parameters are optional (reference layer, CRS, extent and cell size), so from a user point of view a tool should NOT throw an error if none of the optional parameters is defined.

Well, this is because there is no other way to define mutually exclusive parameters. Same approach used in GDAL/OGR algs, e.g. Import into PostGIS, where some parameters are optional and user need to define at least one of them. There is QEP aimed to fix this qgis/QGIS-Enhancement-Proposals#120

If either the reference layer OR the other 3 parameters are mandatory, and if Processing can't handle this scenario, then the tool should be split in two.

API is frozen, splitting algorithm will break scripts and models. Also having yet another two raster calculators IMHO is even more confusing.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Alexander Bruy wrote:

Giovanni Manghi wrote:

ok Alex, let me rephrase: all parameters are optional (reference layer, CRS, extent and cell size), so from a user point of view a tool should NOT throw an error if none of the optional parameters is defined.

Well, this is because there is no other way to define mutually exclusive parameters. Same approach used in GDAL/OGR algs, e.g. Import into PostGIS, where some parameters are optional and user need to define at least one of them.

ummm, ouch... ok. I have to have a look at them then, as it was me that did those tools.

There is QEP aimed to fix this qgis/QGIS-Enhancement-Proposals#120

ok. Too bad, I was thinking this qep was already implemented in qgis3.

If either the reference layer OR the other 3 parameters are mandatory, and if Processing can't handle this scenario, then the tool should be split in two.

API is frozen, splitting algorithm will break scripts and models. Also having yet another two raster calculators IMHO is even more confusing.

yeah I agree and understand, but the python error message is also not very nice.

So I guess this can be closed. Thanks for the explanations.

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Bulk closing 82 tickets in feedback state for more than 90 days affecting an old version. Feel free to reopen if it still applies to a current version and you have more information that clarify the issue.

---

• resolution was changed from to no timely feedback
• status_id was changed from Feedback to Closed

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

On seconds thoughts I think that the very least we can do is to remove the "optional" from the CRS, extent, etc. options... this because they are not optional... if you do not choose CRS,output resolution, extent, etc the tool spits python errors.

---

• version was changed from 3.3(master) to 3.7(master)
• status_id was changed from Closed to Open
• assigned_to_id removed Luigi Pirelli
• resolution was changed from no timely feedback to

[AlisterH]

On seconds thoughts I think that the very least we can do is to remove the "optional" from the CRS, extent, etc. options... this because they are not optional... if you do not choose CRS,output resolution, extent, etc the tool spits python errors.

Unless something has changed in master, you are missing the point. The tool doesn't spit errors if you don't choose them but do choose a reference layer.

[gioman]

Unless something has changed in master, you are missing the point. The tool doesn't spit errors if you don't choose them but do choose a reference layer.

@AlisterH something changed, because before you got those errors even when selecting the "reference layer". It still baffles me why is needed to choose "that" reference layer (if the expression uses just one input why not using it by default) and especially it bothers me (as user) that if I don't select it then I get error messages about missing parameters that are listed as optional.

[AlisterH]

before you got those errors even when selecting the "reference layer"

Well, I don't get them in the current release, and now you are saying you don't in master, so it is working as intended; the original issue appears to be fixed.

[AlisterH]

It still baffles me why is needed to choose "that" reference layer (if the expression uses just one input why not using it by default)

Yes, even if there is more than one input, it would be nice if it tried to use the values for the first layer, or even the intersection of all the layers.

[gioman]

Well, I don't get them in the current release, and now you are saying you don't in master, so it is working as intended; the original issue appears to be fixed.

yes, but then needs to be backported to ltr.

[alexbruy]

Can we close this? As far as I can see now more detailed messages shown in master and 3.4

[gioman]

Can we close this? As far as I can see now more detailed messages shown in master and 3.4

yes