Conversation
|
ask @iuliaL for a proper review of this PR please. |
probably you meant Iulia Rautu, not me 👅 |
|
Salut @irautu-bluesquare ! |
irautu-bluesquare
left a comment
There was a problem hiding this comment.
Nothing to add to the pull request, thanks a lot for structuring this; there was just a previous change which needs to be addressed (see my comment).
There was a problem hiding this comment.
I see that this cell (that was not modified in this pull request) was previously changed, so it doesn't follow the initial logic of the pipeline anymore. And i think it would be useful to retrieve that initial logic (or combine it with the new logic, because it was more flexible.
The part i'm referring to is the importing of the population data, "POP_FILE" (the first one under section "2.2. Load population data"). The initial logic was to allow for user input of population data, and if no data was input by the user, to default back to the last WorldPop extract. So, initially it was:
- if POP_FILE is null => import the latest .tif file from data/worldpop/raw
- if POP_FILE is not null (input by the user): use the file that the user wants
This approach changed sometime along the way and now, if POP_FILE is empty, it stops, throws an error and asks to rerun the WorldPop pipeline. And since the default value of the param POP_FILE is NULL (the starting hypothesis is that the user will not input a file, but use our default file), it does precisely that :D
So, i see two options.
Option 1 (easiest): reuse the original approach (should be available in the history of changes of this file)
Option 2 (more elegant, i think): add the "ask to rerun WorldPop pipeline" that you suggest, to the original approach. So we'd have something like:
- if POP_FILE is not null => it means user has input a file => use that file
- if POP_FILE is null => check if a .tif file exists in data/worldpop/raw and follows the "{COUNTRY_CODE}worldpop_ppp{year_integer}.tif" pattern. Then:
a) if at least one file like that exists => pick the one with the largest value for "year_integer" and use that
b) if no file like that exists => throw the error message that you suggested, asking the user to rerun the WorldPop pipeline
Or if you have an Option 3, lemme know :)
There was a problem hiding this comment.
I like the option 2 better, it is indeed more elegant and make more sense to me. It is more "complete" and cover most scenarios
No description provided.