epiforecasts / socialmixr

R package for deriving social mixing matrices from survey data.
http://epiforecasts.io/socialmixr/
Other
38 stars 11 forks source link

Hugo review round 2 #37

Closed Bisaloo closed 2 years ago

Bisaloo commented 2 years ago

Tests are failing but I'm somewhat suspecting it's not actually my fault. It seems that the change in 7dcfca5fdb3046f6d13e3c32824f0ba0a7d08038 reveals a hidden bug. The age.group variable in contact_matrix() is actually never defined. data.table was quite happy with it being NULL, as set initially but now it's registered as an actual global variable, it doesn't work:

https://github.com/epiforecasts/socialmixr/blob/c83358240dc751602ec4fb30c5dc901590563707/R/contact_matrix.r#L550-L555

This hypothesis is confirmed by the fact that tests pass fine when I revert 7dcfca5fdb3046f6d13e3c32824f0ba0a7d08038, even though it should be an entirely benign change.

This PR may seem quite overwhelming if you look at it as a single change but commits are separated in quite clearly distinct changes so it should be much easier if you review commit by commit.

sbfnk commented 2 years ago

Great stuff, thank you!