-
Notifications
You must be signed in to change notification settings - Fork 20
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
Vignettes #31
Vignettes #31
Conversation
@jannes-m perhaps you can give an update about the status and your further plans for this draft PR? Should it be reviewed already? |
It's been a while so I guess before reviewing I would have to update the vignette so that it reflects the many changes in the code base, great work, by the way 😊👍. See also my reply to #124. |
@florisvdh, could you pls have a look at the qgisprocess.Rmd vignette and let me know if there is anything which we need to add.
Originally, we thought it might be a good idea to write another vignette replicating the use case we have introduced in the RQGIS paper but I have to admit I am a bit unsure about the added value. Thoughts? In any case, first and foremost we should finish this introductory vignette. |
@jannes-m thanks for reviving this! I have looked into Of course PR #142 is best handled first before making new changes here. Further comments (not tackled in PR #142):
Regarding your questions:
I'll look into the other vignette later on. |
Hey @florisvdh,
It would be easier to present no output at all, i.e., just setting
Otherwise, I'll agree with your suggestions and will act on them accordingly ( Today, I have talked to @Robinlovelace, and he agreed that we can just reference the Dockerfile in the basic vignette and link to the according help pages instead of writing an explicit docker vignette. And I also think that I would just delete the other vignette (use_case.Rmd). |
|
qgis_run_algorithm("qgis_alg", args) |>
{\(x) qgis_run_algorithm("qgis_alg2", input = x$OUTPUT)} () I know the lambda function looks odd but this would be imo the most intuitive way of piping. This would also let the user specify the output actually needed for further processing instead of just using the first output object by default. Possibly it would also make |
@jannes-m Indeed, piping is always possible if the user undertakes the effort needed to correctly pass arguments. However |
Hey @florisvdh,
Of course, this might be just personal taste, and I just want to let you know my reasoning. If you still prefer to use P.S.: I forgot the {} in the pseudo code lamba piping example, so I have added them, ah, what the hack, let's try a real world example: library("qgisprocess")
#> Attempting to load the cache ... Success!
#> QGIS version: 3.28.2-Firenze
#> Having access to 1164 algorithms from 6 QGIS processing providers.
#> Run `qgis_configure(use_cached_data = TRUE)` to reload cache and get more details.
#> >>> Run `qgis_enable_plugins()` to enable 1 disabled plugin(s) and
#> access their algorithms: otbprovider
library("terra")
#> terra 1.7.18
dem = rast(system.file("raster/dem.tif", package = "spDataLarge"))
out = qgis_run_algorithm(algorithm = "saga:sinkremoval", DEM = dem,
METHOD = 1, .quiet = TRUE) |>
(\(x) qgis_run_algorithm(algorithm = "saga:sagawetnessindex",
DEM = x$DEM_PREPROC[1], .quiet = TRUE)) ()
#> Argument `SINKROUTE` is unspecified (using QGIS default value).
#> Using `DEM_PREPROC = qgis_tmp_raster()`
#> Argument `THRESHOLD` is unspecified (using QGIS default value).
#> Argument `THRSHEIGHT` is unspecified (using QGIS default value).
#> Argument `WEIGHT` is unspecified (using QGIS default value).
#> Using `AREA = qgis_tmp_raster()`
#> Using `SLOPE = qgis_tmp_raster()`
#> Using `AREA_MOD = qgis_tmp_raster()`
#> Using `TWI = qgis_tmp_raster()`
#> Argument `SUCTION` is unspecified (using QGIS default value).
#> Using `AREA_TYPE = "[0] total catchment area"`
#> Using `SLOPE_TYPE = "[0] local slope"`
#> Argument `SLOPE_MIN` is unspecified (using QGIS default value).
#> Argument `SLOPE_OFF` is unspecified (using QGIS default value).
#> Argument `SLOPE_WEIGHT` is unspecified (using QGIS default value).
# and doing the same using `qgis_pipe()`, though there certainly is a more
# elegant way of doing it
# out_2 = qgis_run_algorithm(algorithm = "saga:sinkremoval", DEM = dem,
# METHOD = 1, .quiet = TRUE) |>
# qgis_result_single() |>
# unclass() |>
# qgis_pipe("saga:sagawetnessindex") Created on 2023-03-24 with reprex v2.0.2 Excellent, this has worked! out = qgis_run_algorithm(algorithm = "saga:sinkremoval", DEM = dem,
METHOD = 1, .quiet = TRUE) |>
qgis_run_algorithm(algorithm = "saga:sagawetnessindex",
DEM = _$DEM_PREPROC[1], .quiet = TRUE) This would be pretty amazing. IMO this would make |
Hi @jannes-m thanks for your involvement and these examples! Let's try to make a comparison (which can evolve), since there are multiple differences:
* that is after #134 is completed So the syntax for qgisprocess/tests/testthat/test-qgis-function.R Lines 56 to 58 in cea42b0
That being said, @paleolimbot is not a fan of piping algorithms at all IIUC, especially because it looses track of intermediate results, which can then not be easily cleaned (if you want to remove temp output files after further reading/processing, you need to pass a Personally I'm in favour of having multiple possibilities. If Two other points that you touched:
So to me several options seem viable and could co-exist. |
Hey @florisvdh, |
Yes, good ideas! Coming week (the 2 weeks after that I may be not much online) I hope to:
If you could have a quick look at PR #143, that would be great. I actually want to publish that vignette (or its first version) soon since I'm referring it in a newsletter of my organization (we supported QGIS financially to improve QGIS expression support in |
Hey @florisvdh, |
Apart from that, it could be an idea to move the handling of a
library(sf)
library(qgisprocess)
filepath <- system.file("gpkg/buildings.gpkg", package = "sf")
read_sf(filepath) |>
dplyr::filter(cat < 20) |>
qgis_run_next_algorithm("native:buffer", DISTANCE = 300) So still looking for some more independent name,
|
But qgis_run_algorithm.qgis_result = function(
.data,
algorithm,
...,
.select = "OUTPUT",
.clean = TRUE,
.quiet = TRUE
) {...} But anyways, this is just me rambling from a bird's eye perspective without ever having touched the underlying source code, so you are definitely in a better position to judge what makes sense and what not and if this complies with the package philosophy or not. |
Thank you for challenging this 🙂. OK, what I learned now is that a method can indeed take a superset of the generic's arguments (e.g. adding But. The dispatch takes place at the level of the generic, so needs an argument at the level of the generic to dispatch by. Hence the argument by which to dispatch cannot belong to |
OK, settling on |
Maybe, I am wrong but I sill think it should work. But instead of guessing, one should try. So if I find the time (but I cannot promise anything, it's really hard to code for fun with three little kids), I'll write a |
Feel free to try! For it to be used when piping a |
Hey @paleolimbot,
I should have started writing the vignettes in another branch right from the beginning instead of editing the master. So here we go. This is still work in progress but since it appears that the multilayer input bug was fixed (#25, qgis/QGIS#40287), I will resume working on the vignettes in the coming weeks.