-
Notifications
You must be signed in to change notification settings - Fork 4
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 structure to foi_index
across the package
#221
base: dev
Are you sure you want to change the base?
Conversation
- Add structure to `get_foi_index()` output - Add structure to `foi_index` in `build_stan_data()`
eab13be
to
011c32c
Compare
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.
Thanks very much, @ntorresd -- this is so much better. A few minor changes then I'm happy for this to go in.
@@ -107,3 +107,28 @@ validate_survey_and_foi_consistency_age_time <- function( #nolint | |||
"not exceed max age in survey_features." | |||
) | |||
} | |||
|
|||
validate_foi_index <- function( |
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 suggest checking that the FOI chunks are consecutive: e.g. that we don't get 1, 1, 1, 2, 2, 2, 1, 1, 3, 3. I think we also need to have indices for all integers up until the max. So we would raise a warning for 1, 1, 1, 3, 3 for instance because it misses a 2.
names(result_age), | ||
must.include = c("age", "foi_index") | ||
) | ||
expect_equal(nrow(result_age), max(survey_features$age_max)) |
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 think there are more things we could check: e.g. max index, all indices represented up until that max index, that we only have contiguous indices. I realise this may seem overkill given the simplicity of this function, but we could imagine changing this function in the future and we'd want to check that it still works as intended.
As pointed out by @ben18785 in #200, the way in which we were indexing the FOI was ambiguous as we were just providing a vector with the indexes (see #206):
rather than an structured object to specify the age/time indexation for estimating the FOI:
Now on, users will need to specify
foi_index
as a data.frame indexing either ages or years (as shown above) rather than as an unstructured list.get_foi_index()
may still be used for this end and its used the same as before with the caveat that the model type must be explicitly specified (e.g.):This PR also adds unit tests for
get_foi_index()
, addressing #220.