Use review: Histo2Hindcast
Hi @eexarchou
Thanks for reviewing the function Histo2Hindcast() for us.
The function is in the branch develop-Histo2Hindcast here. Here are some recommended steps to test it.
- Read the documentation (i.e., the header of the function) to check if there is anything unclear or wrong.
- Find suitable data to test with both s2dverification::Histo2Hindcast and s2dv::Histo2Hindcast.
- One way to test the two functions together without confusion is to assign them with different names first. For example,
Histo2Hindcast_old <- s2dverification::Histo2Hindcast
source('<path_to_new_function>/Histo2Hindcast.R')
Histo2Hindcast_new <- Histo2Hindcast
res_old <- Histo2Hindcast_old(...)
res_new <- Histo2Hindcast_new(...)
- Check if the two results are identical. The dimension order may change after the calculation, and you can use
s2dv::Reorder
to adjust the order back. - Besides the data itself, is there anything to be improved? E.g., the input parameters, the output format, etc.
Here are some points I'd like to ask your opinions.
- The function (both old and new) only deal with sdate = 1. The length cannot be > 1. The old function simply ignores "sdate > 1" data, and the new function returns an error. Do you think it makes sense?
- The old function assumes that all the "sdatesout" have the same month. It doesn't consider the start dates with different months, e.g., c('19901101', '19910501'). The new function takes this into consideration. Which one makes more sense to you?
- The old function only accepts the format 'YYYYMMDD' and the new function also allows 'YYYYMM', since 'day' is not considered in the calculation. Is the modification good?
The review is not urgent, but it would be great to finish within two to three weeks. Please let me know if you have any questions, and thanks again for your help!
Cheers,
An-Chi