-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add statistical tests for infection loop. Closes #52. #60
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make num_infections_end_one_time_unit
a usize
, but otherwise it looks fine to me.
26ba937
to
c3d42ca
Compare
src/infection_propagation_loop.rs
Outdated
context.add_plan_with_phase(1.0, ixa::Context::shutdown, ExecutionPhase::Last); | ||
// Add a bunch of people so we mimic an infinitely large population. | ||
for _ in 0..100 { | ||
context.add_person(()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to unwrap() here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context.add_person(_)
returns Result<PersonId, IxaError>
to return an error in case there are person properties that need to be initialized at start-up that aren't specified!
src/infection_propagation_loop.rs
Outdated
// population) but those people themselves cannot transmit secondary infections. We're going | ||
// to stop those simulations at the end of 1.0 time units and compare the number of infected | ||
// people to the infectious rate we used to set up the simulation. | ||
// We're also going to check the times at which they are infected -- since we only record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate here why infection times are expected to be uniform?
context.add_person(()).unwrap(); | ||
} | ||
// We don't want infectious people beyond our index case to be able to transmit, so we | ||
// have to do setup on our own since just calling `init` will trigger a watcher for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I don't like this but can't think of anything better. Is the goal to keep track of the number of infection attempts as in the expected number of infections in a homogeneous and large completely susceptible population? What's the difference with having two people, one infected, the other one recovered. Then infectious person would keep trying to infect the recovered person over and over again, but they wouldn't be able to transmit to others. Not suggesting to implement this but wondering if there's a more explicit/straightforward way of measuring infection attempts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think this is a great point. You are absolutely right about the goal. I really like your suggestion instead, and that's what I implemented before. I am happy to switch to that implementation (and I think it will be faster because the population size is smaller) but do you think the code is cleaner?
ixa-epi-isolation/src/transmission_manager.rs
Line 355 in 9ee4b69
fn record_infection_times( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ended up being so much faster, thanks Guido!
} | ||
|
||
#[test] | ||
fn test_number_timing_infections_one_time_unit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of this test and would also like to see it as a plot outside of ixa, which I think we can recreate with the transmission chain report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it. Would love to maybe talk with @eqmooring about this.
1ea49ae
to
21d0306
Compare
This is inspired by the tests we had the previous transmission manager (see #51's deleted files) but welcome suggestions for other statistical tests to include.