paleolimbot / ggspatial

Enhancing spatial visualization in ggplot2
https://paleolimbot.github.io/ggspatial
368 stars 35 forks source link

Refactor `stars` functions #97

Closed dieghernan closed 2 years ago

dieghernan commented 2 years ago

This PR is focused on the handling of stars objects, that was not properly working on the dev version. I am aware that this could be a bit tricky to review. Some notes:

Closes #85

paleolimbot commented 2 years ago

Thank you! This is definitely worth doing...be patient as I get the chance to review properly but I appreciate the PR and look forward to getting this merged.

dieghernan commented 2 years ago

Hi @paleolimbot, I assume you are busy, just wanted to know if you had the chance of having a look on this

regards

paleolimbot commented 2 years ago

Thank you for the ping! I'll make sure this is done tomorrow!

paleolimbot commented 2 years ago

Thank you for this PR, and very sorry it fell off my radar. Feel free to ping me more frequently if this ever happens again!

I'm using this reprex to check that this PR does what it says it does, and it works great! Let me know if there's a different reprex I should be checking.

library(ggspatial)
library(stars)
#> Loading required package: abind
#> Loading required package: sf
#> Linking to GEOS 3.9.1, GDAL 3.2.3, PROJ 7.2.1
#> Registered S3 methods overwritten by 'stars':
#>   method             from
#>   st_bbox.SpatRaster sf  
#>   st_crs.SpatRaster  sf
library(ggplot2)

r <- read_stars(system.file('longlake/longlake.tif', package = 'ggspatial'))

# Works
ggplot() +
  layer_spatial(r, lazy = FALSE)

# Also works but does not change the result from the above
ggplot() +
  layer_spatial(r, lazy = FALSE, dpi = 400)

# Doesn't work
ggplot() +
  layer_spatial(r, lazy = TRUE, dpi = 400)

Created on 2022-01-13 by the reprex package (v2.0.1)

I'll go through the files to see if I can spot any errors as well and leave a review.

paleolimbot commented 2 years ago

I'm seeing from the codecov check that there are a few lines in makeContent. where the coverage changed. In particular, it looks like there are a few branches that aren't currently tested. If you're in a position to add tests for those branches I would be grateful (but also feel free to open an issue to remind me to do it later). If I remember correctly it wasn't tested at all before, so this is a huge improvement!

paleolimbot commented 2 years ago

I opened the issue for you! Thank you!

dieghernan commented 2 years ago

So glad to see this! Thanks @paleolimbot

Just a quick note on the use of dpi. If my understanding is correct (and given the description of the parameter):

dpi: if lazy = TRUE, the dpi to which the raster should be resampled

It should only affect on dpi = TRUE. See here a reprex, where it seems to work

library(ggspatial)
library(stars)
#> Loading required package: abind
#> Loading required package: sf
#> Linking to GEOS 3.9.1, GDAL 3.2.1, PROJ 7.2.1; sf_use_s2() is TRUE
library(ggplot2)

r <- read_stars(system.file("longlake/longlake.tif", package = "ggspatial"))

# Works
ggplot() +
  layer_spatial(r, lazy = FALSE)


# Less dpi: Shouldn't do anything, as per the parameter description:
# dpi: if lazy = TRUE, the dpi to which the raster should be resampled
ggplot() +
  layer_spatial(r, lazy = FALSE, dpi = 10)

# This should work since lazy = TRUE
ggplot() +
  layer_spatial(r, lazy = TRUE, dpi = 10)
#> Warning in stars::st_warp(rst, template_raster, use_gdal = TRUE, method =
#> raster_method, : no_data_value not set: missing values will appear as zero
#> values

Created on 2022-01-14 by the reprex package (v2.0.1)