OCHA-DAP / ripc

Download and Tidy IPC and CH Data
GNU General Public License v3.0
2 stars 1 forks source link

possible to filter by multiple years? #13

Open zackarno opened 5 months ago

zackarno commented 5 months ago

Just had a use-case where I wanted to filter ipc_get_areas() by multiple years.

Seems only possible for single years.

Snipped from ipc_get_areas(), but if possible I would imagine logic could be applied to other ipc_get calls as well if

https://github.com/OCHA-DAP/ripc/blob/fa404d0e3ba4a91c6cebc53b79726a132f2db723/R/ipc_get_areas.R#L32-L35

caldwellst commented 5 months ago

So, could be nice to include this functionality. However, I have a concern on the backend for allowing multiple passing of parameters. Say you wanted to allow multiple country and year values. These cases would be easy:

ipc_get_areas(country = "AF", year = 2020)
ipc_get_areas(country = "AF", year = 2020:2022)
ipc_get_areas(country = c("SO", "AF"), year = 2020)

That is because in the underlying script, we could simply use a purrr::map() style call passing in those vectors. However, if you had mismatched lengths of vectors, you'd have to do something different. Maybe using tidyr::complete() to get all combinations of the values. For instance:

ipc_get_areas(country = c("SO", "AF"), year = 2020:2023)

That would fail with any basic purrr::map() setup. At this stage, given that we can just return all values and manually filter after, wondering if the complexity is worth it?

zackarno commented 4 months ago

Yea probably not worth it. The user has good options for post-filtering.

What do you think about just wrapping the filter call - I know it's not elegant or effecient, but still might improve user-experience and I doubt the majority of users would really notice the lag

something to this effect:

ipc_get_areas <- function(..., year){
  if( !is.null(year) & length(year)>1){
       ipc_get_areas(...) |>
          filter(year %in% in year
}
caldwellst commented 4 months ago

So looking into this, again, could be quite complex. For ipc_get_areas(), to have a consistent API, we would want to also handle multiple passes of other parameters: country, period, even id, alongside year. Is possible, but I am not so sure about putting this functionality directly in the package which already does some complex wrangling, and trying to just keep it mirroring the API as much as possible. What do you think Zack? Any thoughts @hannahker?