Closed barrettk closed 2 years ago
kyleam
commented
2 years ago Hmm, @barrettk and @seth127 my gut feeling is that we should hold off on this for this release (just saw that that the issue got tagged for 1.4.0). One consideration is whether we should audit all the places where this comes out of bbi and be more targeted with the replacement, but in general I think it wouldn't necessarily be a bad idea to let the idea sit.
barrettk
commented
2 years ago Hmm, @barrettk and @seth127 my gut feeling is that we should hold off on this for this release (just saw that that the issue got tagged for 1.4.0). One consideration is whether we should audit all the places where this comes out of bbi and be more targeted with the replacement, but in general I think it wouldn't necessarily be a bad idea to let the idea sit.
I'll let @seth127 answer this, as Im only tackling it because we talked about it during our Monday meeting. I will say that the functions I implemented do a great job at fixing this, and so far requires pretty little intervention other than changing a few tests/reference objects and adding a call to model_summary():
res_list <- combine_list_objects(res_list, bbi_list)
res_list <- map_list_recursive(res_list, set_bbi_null) # new line
res_list <- create_summary_object(res_list)It might make more sense to do in bbi, but its not as invasive as I thought it would be if we do it in bbr
kyleam
commented
2 years ago It might make more sense to do in bbi [...]
I'm not really proposing that. I'm suggesting that we might want to be more focused and selective in what we map, unlike the approach here. Or not. And we might want to consider performance hit here when there are lots of values for estimates. Again, I don't know, but this is not a new problem, and my preference would be to hold off on it for 1.4.0.
barrettk
commented
2 years ago Ahh, I just inferred that from
One consideration is whether we should audit all the places where this comes out of bbi
But yeah I take your point. My approach was for sure easier, but I also wanted to make sure it covered everything and had less room for missing cases. In terms of waiting or not I have no comment, but performance wise Im not too concerned. Not really sure how long a list of estimates could become, but I saw no difference when working with the example models (I know they're simpler). If warranted I could benchmark the time difference (before and after this implementation), but it would probably make more sense to do that with a bootstrap run or something?
I'll hold off on writing any tests until Seth responds, but at least the core change (or a version of it) is done (existing tests seem to be passing now).
barrettk
commented
2 years ago @kyleam I've been struggling to write a test for this. Tried a number of things to try to filter the list to only the values that get overwritten. At this point I have little to show for, and to avoid going further down the rabbit hole im wondering if you think updating the summary objects was good enough, or if we still want some type of additional recursive test. Have the following:
map2_list_recursive <- function(.list1, .list2, .func) {
assert_function(.func)
.list_return <- .list1
for (i in seq_along(.list1)) {
value1 <- .list1[[i]]
value2 <- .list2[[i]]
# browser()
if (is.list(value1)) {
.list_return[[i]] <- map2_list_recursive(value1, value2, .func)
} else {
.list_return[[i]] <- .func(.list1[[i]], .list2[[i]])
}
}
.list_return
}
compare_elements <- function(.vals1, .vals2){
map2(.vals1, .vals2, function(.val1, .val2){
if((is.na(.val1) & !is.na(.val2)) | (is.na(.val2) & !is.na(.val1))){
FALSE
}else{
if(.val1 == .val2){
TRUE
}else{
FALSE
}
}
}) %>% unlist()
}updated_list <- map_list_recursive(res_list, set_bbi_null)
is_same <- map2_list_recursive(updated_list, res_list, compare_elements)which leaves the following:
> is_same
$absolute_model_path
[1] TRUE
$run_details
$run_details$version
[1] TRUE
$run_details$run_start
[1] FALSE
$run_details$run_end
[1] TRUE
$run_details$estimation_time
[1] TRUE
$run_details$covariance_time
[1] TRUE
$run_details$postprocess_time
[1] TRUE
$run_details$cpu_time
[1] TRUE
$run_details$function_evaluations
[1] TRUE
$run_details$significant_digits
[1] TRUE
$run_details$problem_text
[1] TRUE
$run_details$mod_file
[1] FALSEProblem is im having trouble filtering this list down to only FALSE, which I planned to compare to an object. Relying on setting values to NULL or capturing an index didnt work, due to the annoying nature of nested lists, and for-loop indexes getting muddied by erasing list elements
barrettk
commented
2 years ago @seth127 @kyleam
Feel like I sunk too much time into this for not a lot of value, but at least we have a working test now. Main downside is that I "had to" (couldnt figure out another way) add another arg to model_summary to be able to test this. After putting a decent amount of time into trying to evaluate a sensible comparison (the tests), I at least wanted to document the option in case it was of value.
On one hand, this new arg would at least allow users to not overwrite these values. If they had a feeling something was wrong, they could inspect it more easily and make sure nothing wonky was going on with NA replacement. God forbid anything weird did happen, we could have a much easier way to figure out what's going on, and the scientists may feel a little better about the change (not that they were worried).
On the other hand, im not sure we want to draw attention to anything bbi related (like BBI_NULL_NUM), and this arg may make some scientists (mostly external in my mind) raise an eyebrow.
... (instead of introducing .overwrite), but wasn't able to get that to work (errors on the call made to bbi). Would like a method of not overwriting BBI_NULL_NUM, and wasn't able to find another option.model_summary arg, but we still want a recursive test (might need some guidance if so)Let me know what you guys think (or whoever responds first)
seth127
commented
2 years ago Talked about this with @barrettk offline. I think that test (and the associated arg/helpers) are overkill. Going with this:
- the model_summary reference objects are enough, and we can just delete all the changes I made in the latest commit
kyleam
commented
2 years ago Just for our information: here's the overhead I see for this current implementation when used on top of model_summaries from gh-527 and the boot directory from the example project (8 core workflow).
without this pr (ce785a40)
> ntimes <- 10
> rbenchmark::benchmark(summary_log(boot_dir), replications = ntimes)
test replications elapsed relative user.self sys.self user.child sys.child
1 summary_log(boot_dir) 10 48.399 1 42.673 3.53 13.671 1.33with this pr (ae8c674c) merged into gh-527
Needed to put this change on top to wire things up:
> rbenchmark::benchmark(summary_log(boot_dir), replications = ntimes)
test replications elapsed relative user.self sys.self user.child sys.child
1 summary_log(boot_dir) 10 63.412 1 57.729 3.514 13.738 1.345That's 10 repeats, so probably between a 1 to 2 second slowdown for 1000 models. That's small compared to the speedup that gh-527 gives.
update
I checked whether lifting the map_list_recursive outside of the model_summaries_concurrent loop would decrease the overhead, but the result is similar.
> rbenchmark::benchmark(summary_log(boot_dir), replications = ntimes)
test replications elapsed relative user.self sys.self user.child sys.child
1 summary_log(boot_dir) 10 63.917 1 58.302 3.34 13.621 1.399Latest commit was a minor refactor so I think its still safe to merge post PR review. Just as a sanity check, here are all the tests passing after the latest commit:
> devtools::test()
ℹ Testing bbr
✔ | F W S OK | Context
✔ | 21 | bbr exec functions [0.6s]
✔ | 13 | checking if models are up to date [0.6s]
✔ | 9 | Collapse columns to string representation [0.7s]
✔ | 73 | Constructing config log from bbi_config.json [2.6s]
✔ | 40 | Copying model objects [0.5s]
✔ | 6 | copy-model-helpers [0.3s]
✔ | 34 | cov-cor [0.7s]
✔ | 35 | Extract model paths from based_on fields [1.5s]
✔ | 34 | Test get_omega, get_sigma, and get_theta functions [1.6s]
✔ | 49 | Build paths from model object [0.4s]
✔ | 7 | model_diff() comparing models [0.4s]
✔ | 19 | Test bbi summary on multiple models [1.0s]
✔ | 105 | Test bbi summary functions [1.6s]
✔ | 73 | Modify attributes of model object [0.4s]
✔ | 25 | Testing function to create or read in model object [0.1s]
✔ | 28 | nm-file [0.2s]
✔ | 29 | nm-join [1.5s]
✔ | 20 | nm-tables [0.2s]
✔ | 30 | Test bbi batch parameter estimate functions [7.0s]
✔ | 11 | Test param_estimates functions [0.7s]
✔ | 43 | test parsing labels for parameter table [1.9s]
✔ | 33 | testing print methods for bbi objects [5.1s]
✔ | 5 | read_bbi_path() helper function
✔ | 50 | Reading NONMEM output files into R [0.9s]
✔ | 41 | Constructing run log from model yaml [1.5s]
✔ | 10 | submit_model(.dry_run=T)
✔ | 14 | submit_models(.dry_run=T) [0.4s]
✔ | 75 | Test creating summary logs [16.4s]
✔ | 9 | Comparing tags between models [0.4s]
✔ | 31 | test_threads(.dry_run=T) [4.9s]
✔ | 7 | test-use-bbi [4.3s]
✔ | 29 | Utility functions for building args, etc. [0.1s]
✔ | 39 | testing a composable workflow and running bbi [95.8s]
══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 154.6 s
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 1047 ]
bbipasses back-999999999(defined here) inbbias a "default" in many cases. Variables are set here inbbr.To do:
model_summary()and other functions to rely on new internal functions for the replacementcheck_run_times(though this might fix itself if the change happens inmodel_summary())BBI_NULL_NUMwithNA_real_for objective functions and checking for that)closes https://github.com/metrumresearchgroup/bbr/issues/520 closes https://github.com/metrumresearchgroup/bbr/issues/518