[Processing] Port GRASS 7.2 algorithm provider #5426
Conversation
|
Yay glad to see this work to reinstate grass! |
|
@medspx I tested a little on MacOS here. I used GRASS 7.0 provided by brew. I needed to set the provider option to
I tried testing a few algs but they do not seem to complete properly. Here is an example of the output. The lag dialog closes before I can catch it so I needed to use icecap to capture it:
|
|
@timlinux Ah! I haven't made tests on MacOSX too. |
|
@timlinux @medspx this somehow recalls me this https://issues.qgis.org/issues/16926 |
|
Unfortunately, it seems to be a GRASS bug: https://trac.osgeo.org/grass/ticket/3170 |
|
@medspx I guess for all Mac users we will have the same problem - I take care not to use spaces in the file paths I use and create myself, but the QGIS configs go into a system defined path which does have spaces :-( This is a side effect of @NathanW2 's (otherwise cool) profiles work.... |
|
I do also wonder how come the grass7_batch_job.sh ends up in that folder? Isn't it something we control? |
|
What about using the newish --exec interface? |
|
@neteler Thanks, it seems to work with the exec interface. I just need to add a commit in this PR! |
|
Thanks for the improvements! Just one quick minor suggestion concerning the "r.out.gdal" usage: it is recommended to add this flag: -m |
| layers = [l for l in self.inputLayers if isinstance(l, QgsRasterLayer)] | ||
|
|
||
| # Use this function to calculate cell size | ||
| def cz(l, cellsize): return max(cellsize, (l.extent().xMaximum() - l.extent().xMinimum()) / l.width()) |
There was a problem hiding this comment.
Maybe I miss something, but why not use rasterUnitsPerPixelX() here?
|
|
||
| def loadRasterLayerFromParameter(self, name, parameters, context, external=True, band=1): | ||
| """ | ||
| """ |
There was a problem hiding this comment.
Missed docstring? Or just remove them
|
|
||
| def loadVectorLayerFromParameter(self, name, parameters, context, external=None): | ||
| """ | ||
| """ |
|
Ok, I've made some changes:
I still need to work on ext/ scripts but the main code (in Grass7Utils and Grass7Algorithm) should now be (approximatively) stable. |
|
@medspx thanks for the updates. I got some new errors now (still on MacOS):
I also still have the issue that All window closes directly after the error so I have to use Licecap to get the above screenshots from a screen record... |
|
@timlinux From your second screenshot, it seems that this an internal error from GRASS. I have sometimes segmentation faults with some data on different algorithms. If you have another clue, just tell me. For the closing windows, have you checked the 'Keep dialog open after running an algorithm' option? |
this should not really matter, meaning that it could be a general processing issue and not something specific to the GRASS provider. |
|
@medspx there's an inbuilt method to auto convert incompatible inputs to a desired format (e.g. shape). I plan on reviewing this later and will advise then. |
there is an option among Processing ones that is not enabled by default (at least in 2.*) and if left unchecked that makes dialogs close after run (even when tools hit some issue and do not produce an output). Not sure if this is the case. |
|
In 3.0 that dialog should stay whenever an error is encountered, regardless of the setting. |
then it maybe this https://issues.qgis.org/issues/16926 it only affects the tools that where added as "external" (python/plugins/processing/algs/grass7/ext/). |
- Improve GRASS detection for all OS. - Use GRASS --exec command. - Unified GRASS batch job method for all OS (easier to maintain). - Handle MS-Windows codepages (for data only, if you have a username with special characters, it will not work). - Better support for filepath normalization. - add -m option to r.out.gdal.
* Fix ParameterRange GUI. * Fix Multiple values Enum support.
* A new createopt textbox has been added to the parameters dialog for algorithms which exports to raster files. * A new metaopt textbox has also been added to the Algorithm parameters dialog. * Raster file format is detected from output filename extension. * GdalUtils has been improved to correctly detect raster formats supported for creation. * QFileDialog for output rasters now display only file filters for supported output raster file formats.
|
any hint how to test it on windows? |
|
@medspx this looks good to me. Given that it has been granted an exemption from the feature freeze, is there anything you think needs to be addressed before it's merged? I'd vote to merge quickly so that we can get wider testing and identify outstanding issues. |
|
Actually one question - have you tried reenabling the grass processing tests to see if the work OK now? |
|
@nyalldawson Here is a short list of what should be done during feature freeze:
I also haven't taken time to launch the tests because it is too early (I'll bet lots of them will fail). |
|
Ok. It's a +1 to merge in its current state from me. I'll let @alexbruy make the final call here! (not sure if he wants to re-review) |
|
@medspx Hi! I started giving GRASS (and SAGA) a go on QGIS3 and noticed the following: there are GRASS description files where the min/max values for a particular parameter got swapped from the 2.18 description file to the 3 one. Example r.watershed 2.18: master: this is causing several symptoms, among the others:
not sure if this is just one case or it is widespead. |
|
@medspx apparently is not the only case, for example v.generalize.simplify 2.18 master: |
|
FWIW: Fedora users can find a version for testing here: |
|
for windows users, it's already available by OSGeo4W. |
|
Questoin: on Linux, how is determined, which GRASS GIS version is used? We tried to change the path to point to my local copy of the upcoming GRASS 7.4.0 version but still 7.2.1 is picked up. Under: |
disregard, I have seen now you fixed (at least the raster ones) here: thanks! |
The path to GRASS binaries is only exposed (in Processing configs) on Windows (and macOS apparently), on Linux is automatically configured (if GRASS is installed). |
|
Doesn't look for grass74 (https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/grass7/Grass7Utils.py#L131) |
right. |
|
ok, please replace "grass73" and "grass73.sh" with "grass74" and "grass74.sh" since 7.3 will never be released. However, 7.4.0 RC1 is due today, followed by the final release soon. |
|
|
Important: currently vector data seem to be handled with v.external by default. Please change that to v.in.ogr since v.external is rather limited and does not work properly with all vector data formats. For a good user experience v.in.ogr or v.import are recommended. See discussion in https://lists.osgeo.org/pipermail/grass-dev/2017-November/086598.html |
ghost
deleted the
Description
This PR adds a ported version of Processing GRASS 7.2 provider...
Checklist
fixes #11111in the commit message next to the description[FEATURE]in the commit message[needs-docs]in the commit message and containt sufficient information in the commit message to be documentedscripts/prepare-commit.shscript before each commitWhat has been done
What needs to be done