afsc-gap-products / akgfmaps

Make AFSC bottom trawl survey maps and retrieve map layers
Other
17 stars 6 forks source link

Update make_idw_stack.R #43

Closed EmilyMarkowitz-NOAA closed 10 months ago

EmilyMarkowitz-NOAA commented 10 months ago

Not sure why this little change (to unlist(unique_groups)[ii]), changed from unique_groups[ii, ]) is making the difference in my code runs (and only sometimes) but without it, I sometimes get the below error at the line I changed (currently 232):

[inverse distance weighted interpolation]
[inverse distance weighted interpolation]
Error in (function (cond)  : 
  error in evaluating the argument 'x' in selecting a method for function 'addAttrToGeom': subscript out of bounds
sean-rohan-NOAA commented 10 months ago

It looks like there's a bug in this PR. Please review the check results.

sean-rohan-NOAA commented 10 months ago

Can you provide an example of a species (or code) where you get this error? The code from #40 and #41 makes it seem like you've been running the code with extrapolation.grid.type = "stars" but this section of make_idw_stack() only runs when extrapolation.grid.type = "sf" or "sf.simple"

EmilyMarkowitz-NOAA commented 10 months ago

The issue wasn't solved when I moved from stars to sf, but rather when the input table was changed from a tibble to data.frame. I didn't realize before that my input data was not a data.frame. Not sure why this issue would occur, but I am leaving a note here in case it comes up later. The mistake was on my end. Thanks!

Reproducible code example

Applying a modified example from https://github.com/afsc-gap-products/akgfmaps/issues/41

when input is a data.frame

... the code works no problem!

str(dplyr::bind_rows(akgfmaps::YFS2017,
                     dplyr::mutate(akgfmaps::YFS2017, YEAR = 999)))

'data.frame': 752 obs. of 7 variables: $ COMMON_NAME: chr "yellowfin sole" "yellowfin sole" "yellowfin sole" "yellowfin sole" ... $ YEAR : num 2017 2017 2017 2017 2017 ... $ HAUL : int 110 109 108 194 199 200 202 198 195 196 ... $ STATIONID : chr "C-18" "B-01" "C-01" "M-27" ... $ LATITUDE : num 55.7 55.4 55.7 59 59.7 ... $ LONGITUDE : num -168 -168 -168 -174 -176 ... $ CPUE_KGHA : num 0 0 0 0 0 0 0 0 0 0 ...

library(akgfmaps)
idw <- make_idw_stack(x = dplyr::bind_rows(akgfmaps::YFS2017,
                                           dplyr::mutate(akgfmaps::YFS2017, YEAR = 999)), 
                      grouping.vars = "YEAR",
                      out.crs = "EPSG:3338",
                      region = "bs.all",
                      extrapolation.grid.type = "sf")

[inverse distance weighted interpolation] [inverse distance weighted interpolation] [inverse distance weighted interpolation] [inverse distance weighted interpolation] Warning messages: 1: attribute variables are assumed to be spatially constant throughout all geometries 2: In st_join.stars(stars::st_rasterize(sf::st_transform(predict(idw_fit, : st_join found 166 1-to-n matches, taking the first match for each of those 3: attribute variables are assumed to be spatially constant throughout all geometries 4: In st_join.stars(stars::st_rasterize(sf::st_transform(predict(idw_fit, : st_join found 166 1-to-n matches, taking the first match for each of those 5: attribute variables are assumed to be spatially constant throughout all geometries

library(ggplot2)
(p1 <- ggplot() +
  geom_sf(data = idw$extrapolation.stack,
          mapping = aes(fill = var1.pred),
          color = NA) +
  scale_fill_viridis_d() +
  facet_wrap(~YEAR))

image

when input is a tibble

... the code fails at the end of the first IDW with the same error I initially reported in this PR

library(akgfmaps)
idw <- make_idw_stack(x = dplyr::bind_rows(akgfmaps::YFS2017,
                                           dplyr::mutate(akgfmaps::YFS2017, YEAR = 999)) %>% 
                        tibble(), 
                      grouping.vars = "YEAR",
                      out.crs = "EPSG:3338",
                      region = "bs.all",
                      extrapolation.grid.type = "sf")

[inverse distance weighted interpolation] [inverse distance weighted interpolation] Error in (function (cond) : error in evaluating the argument 'x' in selecting a method for function 'addAttrToGeom': subscript out of bounds In addition: Warning messages: 1: attribute variables are assumed to be spatially constant throughout all geometries 2: In st_join.stars(stars::st_rasterize(sf::st_transform(predict(idw_fit, : st_join found 166 1-to-n matches, taking the first match for each of those 3: attribute variables are assumed to be spatially constant throughout all geometries

sean-rohan-NOAA commented 10 months ago

Interesting, and thanks for the update.

I don't understand why functionality would differ but I suppose most of my projects use tibbles or data.frames and not both. Very interested in figuring out why there are differences in functionality and I'll try to look into it. Please let me know if you manage to figure it out before I do.

Since it sounds like the issue stems from differences in behavior among object types and not an actual problem with the input, I'd be open to the proposed change if it won't affect the functionality for data.frames. Perhaps another option to consider would be to add a class-type check for x and run x <- as.data.frame(x) if x is a tbl. Either way, I think some sort of change is warranted because it seems like users will expect tibbles and data.frames to have the same behavior, which is completely justified.

EmilyMarkowitz-NOAA commented 10 months ago

I also don't know why a data.frame would behave differently from a tibble here. Sounds good and agreed: instead of my suggested fix, I would also vote for adding something like x <- as.data.frame(x) in the code. I think that is a more universal fix. Let me know if you want me to create a new PR with what we've discussed, but I suspect you'd prefer to simply integrate this fix on your end into your next release, right?

sean-rohan-NOAA commented 10 months ago

I'll take care of it shortly. Thanks for bringing this to my attention and figuring out what was causing the error.

sean-rohan-NOAA commented 10 months ago

@EmilyMarkowitz-NOAA Addressed by #99 (version 3.3.1)