Closed seth127 closed 2 years ago
We'll also want to add tests obviously, but I don't think we need to make any changes to requirements because we can just put BBR-RNLG-005
on the new tests and it will be covered under this existing requirement.
I could see some weird behavior if the user has a tag that corresponds to the run ID of a model (that doesn't have that tag) ...
Yeah, as I started reading I wondered what you would propose for distinguishing tags vs run IDs (e.g., split .include
into .include_ids
and .include_tag
, or keep .include
and allow so substructure to disambiguate)...
... but that would be a weird edge case and honestly the failure mode (some unexpected models being included) is pretty explainable and probably totally fine.
... but that seems fair enough.
@callistosp @timwaterhouse @kylebaron @janelleh @sonokokawa @curtisKJ any thoughts on this? It seems like a pretty clearly good move to me, but I want to make sure I'm not missing some subtlety in how folks would use it.
Imagine you have these models (I'm showing a fake run_log()
output, but only showing the relevant columns):
run | tags |
---|---|
100 | base, one_cmpt |
101 | base, one_cmpt |
102 | base, two_cmpt |
103 | covariate, two_cmpt |
104 | covariate, two_cmpt |
105 | covariate, two_cmpt |
106 | covariate, two_cmpt, prop_ruv |
107 | covariate, two_cmpt, prop_ruv |
108 | final, two_cmpt |
Here is the intended behavior for a few different cases:
run_log(..., .include = c(101, 105))
run | tags |
---|---|
101 | base, one_cmpt |
105 | covariate, two_cmpt |
(this is essentially just a numeric vector that gets turned into a character vector under the hood)
run_log(..., .include = 102:105)
run | tags |
---|---|
102 | base, two_cmpt |
103 | covariate, two_cmpt |
104 | covariate, two_cmpt |
105 | covariate, two_cmpt |
run_log(..., .include = "covariate")
run | tags |
---|---|
103 | covariate, two_cmpt |
104 | covariate, two_cmpt |
105 | covariate, two_cmpt |
106 | covariate, two_cmpt, prop_ruv |
107 | covariate, two_cmpt, prop_ruv |
run_log(..., .include = c("final", "prop_ruv"))
run | tags |
---|---|
106 | covariate, two_cmpt, prop_ruv |
107 | covariate, two_cmpt, prop_ruv |
108 | final, two_cmpt |
(this one is a little weird, but I still think it's fine)
run_log(..., .include = c("102", "108", "prop_ruv"))
run | tags |
---|---|
102 | base, two_cmpt |
106 | covariate, two_cmpt, prop_ruv |
107 | covariate, two_cmpt, prop_ruv |
108 | final, two_cmpt |
love love love. this would be so useful. I think a lot of people filter the runlog any way, but putting this in as an argument is a great idea (especially if it speeds up runtime of run_log()
)
Just a thought - I dont think we should include tags and run numbers in the same argument, unless its like bbi_args()
like @kyleam suggested (i.e. a named list). Given that we are operating on two different objects (its not a dataframe by the time the filtering happens), the filtering would be an and statement, rather than an or statement (which we want). The filtering cant happen sequentially if we want it to be an or statement (at least without having code that adds values back in)
(I spent a little time looking into this, though now that I come to respond, I see you've already implemented it.)
I think we want to stick with a single .include
arg, for UX reasons (which we can get into in more detail if you want to).
I took some time on it because you make a good point that this means you actually have to read the YAML files instead of just looking at the names of them. I did some timing on them (details below) and I don't think the time to do that is really much of a big deal. TL;DR it took less than 2 seconds to read in 1000 models that each had 50 tags on them.
Note that for the benchmark()
calls, it's doing it 10x, so you can divide the times reported by 10.
This is a proposal to modify the (still currently unreleased) functionality that was added in #484 and originally proposed in #477 so that a user could pass a vector of either run ID's (current behavior) or tags to the
.include
argument.In discussions offline with some users, filtering on tags is a very useful thing to do in many cases. This is currently possible but, as we demonstrate in a vignette, requires writing some
tidyverse
code that is a bit convoluted and requires knowledge of the underlying structure of the run log (specifically thattags
is a list column and must be mapped over).Proposal
All
*_log()
functions will have an.include
argument. Users can pass a vector and the resulting log tibble will only included models that satisfy one of the following:.include
.include
I am trying to think through if there is any problem mixing-and-matching (i.e. passing a vector that contains both run IDs and tags to filter on), but I think it should be fine. I could see some weird behavior if the user has a tag that corresponds to the run ID of a model (that doesn't have that tag), but that would be a weird edge case and honestly the failure mode (some unexpected models being included) is pretty explainable and probably totally fine.
Implementation
I think we'll just need to modify the code here (and accompanying docs here).