mapme-initiative / mapme.biodiversity

Efficient analysis of spatial biodiversity datasets for global portfolios
https://mapme-initiative.github.io/mapme.biodiversity/dev
GNU General Public License v3.0
33 stars 7 forks source link

TEOW Ecoregion contains unexpected results #101

Closed karpfen closed 2 years ago

karpfen commented 2 years ago

mapme.biodiversity version: 0.2.1

When calculating the ecoregions of two regions where one of them lies outside of the TEOW data, the package writes the ecoregion and area of the first region into both result regions. The reproducible example below shows this behavior.

library(sf)
library(mapme.biodiversity)

coords <- data.frame (
  lon = c(-43.59019, -44.42497),
  lat = c(-22.75776, -25.50908)
)

pts <- st_as_sf(coords, coords = c("lon", "lat"), crs = 4326)
x <- st_buffer(pts, c(1000, 10000))

res <- x %>% init_portfolio(2000:2021,
                     cores = 1,
                     verbose = TRUE) %>%
  get_resources(
    resources = c("teow")
  ) %>% calc_indicators("ecoregion")

unlist(res$ecoregion)[c(2,4)]
#               area               area 
# "319.046908635397" "319.046908635397"

st_area(x)
## Units: [m^2]
# [1]   3190469 318064610
goergen95 commented 2 years ago

Thanks for reporting! I won't have time to dig into this for about two weeks. I would happily except a PR if you find a solution.

.calc_ecoregions() should return NA if no intersection is found. I thus expect the bug to occur somewhere here:

https://github.com/mapme-initiative/mapme.biodiversity/blob/25fa3d195f7cc07febe47010f9674da999a31672/R/calc_indicator.R#L112-L123

What this is supposed to do is to just row bind the data frames if no Polygon returned NA or copy the column names and fill in NA for Polygons where NA has been returned. I hope this can get you started in finding a fix.

karpfen commented 2 years ago

Cool, thanks for the hint! I'll start working on a PR next week, I'll be in touch.

karpfen commented 2 years ago

@goergen95 I just wrote a fix on my fork (s. here), however before doing a PR I want to make sure it works and write a unit test that covers the error I described above.

However, I'm unfamiliar with the except_snapshot tests and they seem to fail even when I just clone the repo and run devtools::test() without any changes to the code. Are there any special requirements for the enivonment I have to consider in order to be able to test and check?

My Setup: R version 4.2.1 devtools: 2.4.4 testthat: 3.1.4

karpfen commented 2 years ago

Update: I read up a bit on snapshot tests now and figured out what the problem with the unit tests was. Some of the snapshots in the repo contain likes like "# i Use print(n = ...) to see more rows, and colnames() to see all variable names" On my machine these weren't generated, that's what caused the unit tests for me to fail. But knowing that I was able to test and check my changes. I'll send a PR.

goergen95 commented 2 years ago

Update: I read up a bit on snapshot tests now and figured out what the problem with the unit tests was. Some of the snapshots in the repo contain likes like "# i Use print(n = ...) to see more rows, and colnames() to see all variable names" On my machine these weren't generated, that's what caused the unit tests for me to fail. But knowing that I was able to test and check my changes. I'll send a PR.

Setting options(pillar.advice = TRUE) before running tests should do the trick. This is done internally here:

https://github.com/mapme-initiative/mapme.biodiversity/blob/25fa3d195f7cc07febe47010f9674da999a31672/tests/testthat.R#L1-L7

goergen95 commented 2 years ago

Could you please try and install dev version via remotes::install_github("mapme-initiative/mapme.biodiversity", ref = "101-teow-ecoregion-contains-unexpected-results") and check if that fixes your issue?

karpfen commented 2 years ago

Just tried it, it works as expected now, thanks!

goergen95 commented 2 years ago

thanks for the quick feedback. the changes are now merged into main.