Bug report #19553
QGIS raster calculator: better handling of optional parameters and error message
| Status: | Open | ||
|---|---|---|---|
| Priority: | Normal | ||
| Assignee: | - | ||
| Category: | Processing/QGIS | ||
| Affected QGIS version: | 3.7(master) | Regression?: | No |
| Operating System: | Easy fix?: | No | |
| Pull Request or Patch supplied: | No | Resolution: | |
| Crashes QGIS or corrupts data: | No | Copied to github as #: | 27380 |
Description
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.
History
#1
Updated by Alexander Bruy over 5 years ago
- Priority changed from High to Normal
- Category changed from Raster Calculator to Processing/QGIS
- Status changed from Open to Feedback
This is not error. Raising QgsProcessingExcpetion is a normal Processing way to report errors like this, others algorithms behaves in the same manner.
#2
Updated by Giovanni Manghi over 5 years ago
- Status changed from Feedback to Open
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?
#3
Updated by Luigi Pirelli over 5 years ago
- Assignee set to Luigi Pirelli
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.
#4
Updated by Alexander Bruy over 5 years ago
- Status changed from Open to Feedback
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.
#5
Updated by Giovanni Manghi over 5 years ago
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).
#6
Updated by Alexander Bruy over 5 years ago
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.
#7
Updated by Giovanni Manghi over 5 years ago
- Subject 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? changed from Yes to No
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.
#8
Updated by Alexander Bruy over 5 years ago
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 https://github.com/qgis/QGIS-Enhancement-Proposals/issues/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.
#9
Updated by Giovanni Manghi over 5 years ago
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 https://github.com/qgis/QGIS-Enhancement-Proposals/issues/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.
#10
Updated by Jürgen Fischer about 5 years ago
- Resolution set to no timely feedback
- Status changed from Feedback to Closed
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.
#11
Updated by Giovanni Manghi about 5 years ago
- Affected QGIS version changed from 3.3(master) to 3.7(master)
- Status changed from Closed to Open
- Assignee deleted (
Luigi Pirelli) - Resolution deleted (
no timely feedback)
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.