metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
21 stars 2 forks source link

Patch/boot model path _not_ from RDS or JSON #698

Closed seth127 closed 1 month ago

seth127 commented 1 month ago

This is necessary because the RDS and JSON have the absolute path on the system where it was originally run. This fails if someone is trying this on a different system. In other words, when you try to load a summary of something that someone else has run, then the paths that are loaded and displayed with be wrong (and part of the print method fails because of this).

Notice the RDS it's loading is on `sethg` system, but the "Based On" paths are from `kyleb`'s system (who actually ran the models). And then, when it tries to read in the model in the print method, `.boot_run[[ABS_MOD_PATH]]` returns the `kyleb` path and fails. ```r > bs <- summarize_bootstrap_run(br) Reading in bootstrap summary: /data/home/sethg/projects/this-repo/models/pk/106-boot/boot_summary.RDS > bs ── Based on ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── Model: /data/home/kyleb/project.mrg/this-repo/models/pk/106.ctl Dataset: /data/home/kyleb/project.mrg/this-repo/data/derived/pk.csv ── Run Specifications ───────────────────────────────────────────────────────────────────────────────────────────────────────────── • Stratification Columns: STUDY • Seed: 1234 Error in read_model(.boot_run[[ABS_MOD_PATH]]) : Assertion on 'yaml_path' failed: File does not exist: '/data/home/kyleb/project.mrg/this-repo/models/pk/106-boot.yaml'. 9. stop(simpleError(sprintf(msg, ...), call.)) at helper.R#2 8. mstop("Assertion on '%s' failed: %s.", var.name, res, call. = sys.call(-2L)) at makeAssertion.R#44 7. makeAssertion(x, res, .var.name, add) at checkmate#1 6. checkmate::assert_file_exists(yaml_path) at new-model.R#96 5. read_model(.boot_run[[ABS_MOD_PATH]]) at model-status.R#119 4. bootstrap_is_cleaned_up(x) 3. isTRUE(bootstrap_is_cleaned_up(x)) at print.R#313 2. print.bbi_nmboot_summary(x) 1. (function (x, ...) UseMethod("print"))(x) ```

Note that I also snuck bac2e4428750929ddeb70c8b85d2ac399df44070 into here, which is totally unrelated. It is a small change to facilitate a new style of diagnostics template and we keep forgetting to add it. Now it is in.

seth127 commented 1 month ago

@barrettk I noticed this when trying to load cleaned up bootstrap output that Kyle Baron had run. I can't think of an easy way to test any of this. We could run a bootstrap in a temp dir, clean it up, copy it somewhere else, and then try to load it. But that all seems a little extreme.

Either way, let me know what you think of this approach, and you think there's any reasonable testing to add.

barrettk commented 1 month ago

@seth127 Good catch. I discovered that we cant copy a fully run bootstrap model for the use of testing, as the spec file will still reference the original models. A cleaned up model is different though; that should work in this case. We could add one to the end of the bootstrap tests before we delete the model. I have some uncommitted simulation updates I want to finish but can add that test today if you'd like.

Edit: What I dont know though: do we want any specific handling for those problematic paths you add ("<not found>")? Specifically within param_estimates_compare() I was thinking (The only function that uses a bootstrap summary object). I didn't test it out on this branch, but the current handling there may be sufficient

barrettk commented 1 month ago

@seth127 going to merge this now in preparation for rebasing the simulation branch. We can decide if we want to add a test for this case there.

seth127 commented 1 month ago

That works for me. Thanks @barrettk

In terms of this question:

do we want any specific handling for those problematic paths you add ("")?

My thinking was that, if we can't find the parent model, then any downstream functions that need it will have to fail. It would probably be good to add some special error handling, so that the message can be more informative, but that can probably be added later. Let me know if you have other thoughts on that.