#1166 Add support for zero-padded months in SWY #1697
Conversation
…to new function _get_monthly_file_lists and altered regex to accept zero-padded month numbers
There was a problem hiding this comment.
Hi @claire-simpson , this looks like a great solution! A nice convenient little function + unit test. I had a few fairly minor comments that perhaps we can discuss, but this looks great overall.
| for mo in range(1, n_months+1)] | ||
| padded_precip_dir_list = [ | ||
| os.path.join("/fake/path", "Precip" + str(mo).zfill(2) + ".tif") | ||
| for mo in range(1, n_months + 1)] |
There was a problem hiding this comment.
This is a very nice self-contained unit test!
Do you think it's worth constructing one of these lists with zero-padded numbers and one without? To demonstrate that _get_monthly_file_lists should accommodate both?
We could also consider a test that asserts a ValueError is raised in certain cases, since the function is designed with those cases in mind too.
There was a problem hiding this comment.
I believe the other tests all use non-zero-padded file names so this concept does get tested - but maybe its still good to have an explicit test for this with the new regex? Happy to add this.
As for the fail cases - I'm thinking double zeros (i.e. raising ValueError on precip001.tif). Did you have anything else in mind?
| @@ -659,33 +659,15 @@ def execute(args): | |||
| output_align_list = [ | |||
| file_registry['lulc_aligned_path'], file_registry['dem_aligned_path']] | |||
| if not args['user_defined_local_recharge']: | |||
| precip_path_list = [] | |||
| et0_path_list = [] | |||
|
|
|||
| et0_dir_list = [ | |||
| os.path.join(args['et0_dir'], f) for f in os.listdir( | |||
| args['et0_dir'])] | |||
| precip_dir_list = [ | |||
There was a problem hiding this comment.
Do you think it makes sense to do these list comprehensions within the _get_monthly_file_lists function? It seems like logically they are closely related to the rest of the code in the function, so it might be nice to encapsulate it there. And then the function would take args['et0_dir'] as a direct input.
|
|
||
| for data_type, dir_list, path_list in [ | ||
| ('et0', et0_dir_list, et0_path_list), | ||
| ('Precip', precip_dir_list, precip_path_list)]: |
There was a problem hiding this comment.
Now that we have this convenient function to use, what do you think about having the function operate on only one directory, and return one list of filepaths? In other words, instead of using this for loop here, we make two separate calls to _get_monthly_file_list from up in execute.
There was a problem hiding this comment.
Yes I like this, will update
Description
Updated the regex to allow users to have zero-padded month numbers in the monthly precipitation and evapotranspiration files. Now, filenames like et0_01.tif and precip01.tif are permitted.
Fixes #1166
Checklist