epiforecasts / ringbp

Simulate infectious disease transmission with contact tracing
http://epiforecasts.io/ringbp/
Other
15 stars 10 forks source link

Add unit tests #43

Closed timcdlucas closed 11 months ago

timcdlucas commented 4 years ago

Hello,

Firstly, thank you for making this code so open, well organised and well documented. It's really appreciated.

I've just started working with Petra via the RAMP programme. We're talking about a project on assessing the effectiveness of contract tracing as we release lockdown and other measures. So we're looking at using this code as a starting point. As the project progresses I imagine I may well talk to you further either about the model or about shared authorship if the analysis ends up relying strongly on your code.

As a way to get to know the code better I started writing unit tests and making some other edits. I can imagine you might not want to merge them into this repo as the repo is so closely linked to the lancet paper. But a PR seemed an appropriate way to say hello and let you see the tests that I have written. If you're still working on similar models perhaps these unit tests would be useful in a different repo for example. I don't know.

I intend to continue working with this code. I may refactor the code and then write more tests for example. Happy to PR any further work to this repo or a similar repo if that's useful.

Anyway, thanks again, Tim

jhellewell14 commented 4 years ago

Hello Tim, nice to meet you!

We (me and @seabbs) have already been discussing the potential of using this code to answer the after-lockdown contact tracing question but decided that we have too much work to do. So it's nice to see someone else picking this code up.

I'm very happy for you to build on the package and would be keen to support the work going forward.

We have a "paper" version of the repository hosted somewhere else so I'm happy to merge these changes (pending a review by @seabbs) here and we will put the previous code as a github release and you can build on top of it.

timcdlucas commented 4 years ago

Hi both,

"It would be good to see more extensive and unit based tests." Totally agree. But I think I'll have to refactor the code to do that. I think I will do that, but it will take a while.

"decided that we have too much work to do." I guess this is exactly what RAMP is for! If you were already thinking about it it makes sense for you to be as involved as you wish with me/others in my lab doing the leg work.

I'll ask some further questions in the specific commit comments. But I know you're busy so please feel no rush to deal with any of this. I'll happily carry on working on my fork anyway.

seabbs commented 4 years ago

Hi Tim,

Great to have you reviewing/improving the code/expanding the research. As Joels says this is very much in the direction we were thinking of looking (if I understand what you are looking at) so sounds like a good idea to join forces!

In terms of unit tests, I agree more modularity in the actual functions would help for testing but think even with the current set up it is possible to test that certain behaviour does what it says on the tin (not that you need to write these tests).

Sam

sbfnk commented 11 months ago

@timcdlucas it may have taken a few years but I we're currently looking into splitting up the repo into a paper repo and an eventually more general R package - for which these tests are very useful! I've polished things a bit and added you as a package co-author. I hope you're ok with this, please do shout if not.

timcdlucas commented 11 months ago

Totally fine by me. It was an interesting flash back to see these pop up in my email! Tim

seabbs commented 11 months ago

Woo yes a blast from the past. Thanks again for writing these @timcdlucas and sorry they didn't get merged in sooner!