mrc-4339: Add a state saver object to configure returned years #12
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
==========================================
+ Coverage 98.11% 98.41% +0.30%
==========================================
Files 9 10 +1
Lines 688 757 +69
==========================================
+ Hits 675 745 +70
+ Misses 13 12 -1
☔ View full report in Codecov by Sentry. |
| Tensor4<real_type> art_initiation; | ||
| Tensor3<real_type> hiv_deaths; | ||
|
|
||
| OutputState(int age_groups_pop, |
There was a problem hiding this comment.
most these values, defining the dimensions, are probably subsets of the model parameters? do you want to pass in the parameters object here, or create some sub-struct within the object so that this call signature does not grow unmanageably, and also that a state saver and its underlying model run are always compatible?
Do we imagine that all of these are always wanted?
There was a problem hiding this comment.
Yes they are, so these are all the dimensions of our state-space. We've started thinking about splitting this out in https://github.com/mrc-ide/frogger/pull/11/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R51 as part of that PR I will update the names and also create some sub-struct for the parameters. If it looks ok I would wait until that PR is pinned down and use whatever struct comes from that. I'll add a TODO note here into the README
inst/include/state_saver.hpp
Outdated
| } | ||
| } | ||
|
|
||
| OutputState get_full_state() const { |
There was a problem hiding this comment.
consider returning const OututState& here
src/frogger.cpp
Outdated
| @@ -54,13 +54,19 @@ leapfrog::TensorMap1<int> get_age_groups_hiv_span(const Rcpp::List projection_pa | |||
| return age_groups_hiv_span; | |||
| } | |||
|
|
|||
| std::vector<int> parse_output_steps(Rcpp::NumericVector output_steps, int proj_years) { | |||
There was a problem hiding this comment.
what is being parsed here? why is proj_years an input that is not used? should you also verify that these are (strictly) increasing?
There was a problem hiding this comment.
We don't need proj_years - good catch. I think that was just some old code left around as I originally had this interface take a time step and was then building output_steps from the time step and proj_years. By parsing here I just really mean converting from R type to C++ type. I've tried updating that name, this is now transform_output_steps and I've updated the other functions similar to this one which are also converting from R type to C++ type, possibly doing some validation on the way.
I can check they are increasing but I think this will work atm even if they aren't. If someone passed in seq(60, 0, -1) it would return with time backwards if someone really wanted. I can explicitly disallow if you think it makes interface tidier
This PR will enable callers to save output from more than 1 timestep. The interface I've gone for here is users setup a vector of indices they want to output e.g.
0:60for all years. You initialise theStateSaverwith this setup then that handles allocating the memory and copying output after every time step into output. I wonder if that controlling of whether to save a particular year should move into the controlling process, I think it might be a bit more obvious what it is doing if we move it there.