Gce minimum report #10
Conversation
There was a problem hiding this comment.
Thanks, @confunguido. I have one big comment on the need to use add_periodic_plan_with_phase here to ensure the report is consistently reporting out at the same time. The rest of my comments are mainly stylistic. I did not review the population_loader in this PR since that is the subject elsewhere.
|
|
||
| context | ||
| .report_options() | ||
| .overwrite(true) |
There was a problem hiding this comment.
I think whether to overwrite the report or not should be in the input.json. What do you think?
There was a problem hiding this comment.
nevermind, we should use command line args instead. i can do tha
src/parameters_loader.rs
Outdated
| pub max_time: f64, | ||
| pub seed: u64, | ||
| pub r_0: f64, | ||
| pub infection_duration: f64, | ||
| pub generation_interval: f64, | ||
| pub report_period: f64, | ||
| pub synth_population_file: String, | ||
| pub output_file: String, |
There was a problem hiding this comment.
What do you think about a more descriptive name for this particular report output file (maybe "population_periodic_report"?) since I assume we'll have multiple output files in the future?
src/periodic_report_population.rs
Outdated
| }); | ||
|
|
||
| context.add_report::<PersonReportItem>(¶meters.output_file)?; | ||
| context.add_plan(0.0, move |context| { |
There was a problem hiding this comment.
Sorry, you need to use the add_periodic_plan_with_phase where phase = ExecutionPhase::Last here. See https://github.com/CDCgov/ixa/blob/c8d9da5c7863e3e233b1531055db34dfdd38f199/examples/time-varying-infection/periodic_report.rs#L59 for an example. This syntax makes periodic reports easier and ensures that the report is telling you the state of the world at the end of a given time!
| context | ||
| .report_options() | ||
| .overwrite(true) | ||
| .directory(PathBuf::from(output_dir)); |
There was a problem hiding this comment.
Nit: I think I would use a clone when feeding in the output_dir to ensure that the value being fed in is a PathBuf. As I understand, this is basically taking a PathBuf, referencing it so it turns into a Path, and then reconverting it to a PathBuf. I wonder if it is just more parsimonious to use args.output_directory.clone() in the arg to this function and make output_dir a PathBuf.
src/periodic_report_population.rs
Outdated
| create_report_trait!(PersonReportItem); | ||
|
|
||
| fn send_population_report(context: &mut Context, report_period: f64) { | ||
| let population_data = context.get_data_container_mut(PopulationReportPlugin); |
There was a problem hiding this comment.
Could this be get_data_container without the mutable reference instead? That would also prevent the clone in the next line I think.
| let population_data = context.get_data_container_mut(PopulationReportPlugin); | ||
|
|
||
| let current_census_set = population_data.census_tract_set.clone(); | ||
| for age_it in 0..100 { |
There was a problem hiding this comment.
Do you think it's worthwhile to try to source the age bounds as an input? Potentially from inputs.json (though I really don't love that) but ideally from the population or the synth pop file? Doesn't have to be addressed or discussed now, just thinking about it for more complicated properties.
There was a problem hiding this comment.
I wouldn't be opposed to hard-coding age as MAXAGE or something like that as a static variable. Not sure that it needs to be in the input, but yeah we can discuss for next iteration.
src/periodic_report_population.rs
Outdated
| use std::collections::HashSet; | ||
|
|
||
| #[derive(Serialize, Deserialize, Clone, PartialEq)] | ||
| struct PersonReportItem { |
There was a problem hiding this comment.
Nit: I think this should be called PopulationReportItem or something. I also think the columns should be in the order of time, census_tract, age, population based on the way the iteration happens.
| person_record: &PeopleRecord, | ||
| ) -> Result<PersonId, IxaError> { | ||
| let tract: usize = String::from_utf8(person_record.homeId[..11].to_owned()) | ||
| .expect("Home id should have 11 digits for tract + home id") |
There was a problem hiding this comment.
Home ID should have 15 digits, the census tract itself is 11 digits and home ID subcomponent is four.
There was a problem hiding this comment.
I would let this to be handled in the population skeleton branch
confunguido
force-pushed
the
gce_minimum_report
branch
from
d089252 to
3faeecf
Compare
confunguido
force-pushed
the
gce_minimum_report
branch
from
c3c9b29 to
0463903
Compare
|
Closing to reopen in population skeleton branch |
Provides a skeleton for a report. Also includes changes from the pop loader so probably need to land that first.