Closed caldwellst closed 7 months ago
Thanks for this, this is great! Some responses:
Full disclosure, I have not made it through the entire thing yet, but I did just notice something that I thought would be worth me taking a pause to make sure I understand. I'm not sure why the return of ipc_get_population()$areas is so different than ipc_get_areas()? For example, it looks like unique(ipc_get_population()$areas$condition) returns just "A". I know ipc_get_population() calls a different resource and then does a bunch of wrangling to get the $areas data.frame, but shouldn't these data sets be related to one another? maybe it's my misunderstanding of the API call.
Yeah, the IPC API has public, simplified endpoints (one of which is areas
) and the developer, advanced endpoints, one of which is population
. From the IPC documentation:
- Calls listed below in the public category return more simplified data and encourage the use of terms more familiar to a non-expert user (e.g., 'country' instead of 'analysis ID'). They provide only the most recent data available, returning the “currently” (as of today) valid period of analysis (regardless of it being current or projected) or the most recent period if all periods are now in the past (projection or second projection);
- Calls in the developer category give more complete data, including access to all validity periods and rely on analysis IDs for more advanced data exploration.
Hence why we have the difference in the data.
what do you think of storing a sample data set with the package? My idea is that the sample data set would contain a sample data.frame of each of the "raw" data sets returned by the different API calls. This could be nice for unit-testing, and potentially examples and could also be kept up to date as required by changes.
Really good idea. Will make an issue and can be something we put in there eventually to ensure that no changes are made to the API that aren't caught in the package, and when they are caught, then we know we need to update it. Won't put in this PR obviously as that would be a bit too much at once.
Thanks again! Have addressed the comments so far. Let me know what you think.
Thanks again! Have addressed the comments so far. Let me know what you think.
great yes , all make sense. I focused most of my effort on ipc_get_population()
so far, will spend a shorter time going through the rest now!
reviewed the rest- looks good to me!
Really nice package i learned some cool tricks from.
ll make a branch to incorporate some ipc data sets
So good, thanks man, look forward to your testing branch!
Full restructure of the API! The
NEWS.md
contains the gist of the changes, but happy to chat about it!