Fix check_run_times and model_summary tests

barrettk commented 2 years ago

This PR addresses bbr changes corresponding to this bbi PR: https://github.com/metrumresearchgroup/bbi/pull/270#event-6942798794

closes https://github.com/metrumresearchgroup/bbr/issues/509

barrettk commented 2 years ago

Note that for tests to currently pass, you have to 1) install the latest version of bbi via bbr::use_bbi("/data/apps/bbi", .version = "v3.2.0-alpha.1") and 2) rebase your branch to main if you have already checked out this branch (if you haven't you should be ok)

Will work on making sure its backwards compatible with older versions of bbi as well

On my end all tests pass with the new version of bbi:

> devtools::test()
ℹ Loading bbr
ℹ Testing bbr
✔ | F W S  OK | Context
✔ |        21 | bbr exec functions [0.9s]                                                                                                                           
✔ |        13 | checking if models are up to date [0.6s]                                                                                                            
✔ |         9 | Collapse columns to string representation [0.5s]                                                                                                    
✔ |        73 | Constructing config log from bbi_config.json [2.1s]                                                                                                 
✔ |        40 | Copying model objects [0.5s]                                                                                                                        
✔ |         6 | copy-model-helpers [0.3s]                                                                                                                           
✔ |        34 | cov-cor [0.6s]                                                                                                                                      
✔ |        35 | Extract model paths from based_on fields [1.3s]                                                                                                     
✔ |        49 | Build paths from model object [0.4s]                                                                                                                
✔ |         6 | model_diff() comparing models [0.4s]                                                                                                                
✔ |        19 | Test bbi summary on multiple models [0.7s]                                                                                                          
✔ |       101 | Test bbi summary functions [0.8s]                                                                                                                   
✔ |        73 | Modify attributes of model object [0.4s]                                                                                                            
✔ |        19 | Testing function to create or read in model object [0.1s]                                                                                           
✔ |        28 | nm-file [0.2s]                                                                                                                                      
✔ |        29 | nm-join [0.9s]                                                                                                                                      
✔ |        20 | nm-tables [0.2s]                                                                                                                                    
✔ |        30 | Test bbi batch parameter estimate functions [6.4s]                                                                                                  
✔ |        11 | Test param_estimates functions [0.5s]                                                                                                               
✔ |        43 | test parsing labels for parameter table [1.6s]                                                                                                      
✔ |        33 | testing print methods for bbi objects [4.0s]                                                                                                        
✔ |         5 | read_bbi_path() helper function                                                                                                                     
✔ |        50 | Reading NONMEM output files into R [0.7s]                                                                                                           
✔ |        41 | Constructing run log from model yaml [1.3s]                                                                                                         
✔ |        10 | submit_model(.dry_run=T)                                                                                                                            
✔ |        14 | submit_models(.dry_run=T) [0.3s]                                                                                                                    
✔ |        75 | Test creating summary logs [7.7s]                                                                                                                   
✔ |         9 | Comparing tags between models [0.2s]                                                                                                                
✔ |        23 | test_threads(.dry_run=T) [4.1s]                                                                                                                     
✔ |         7 | test-use-bbi [3.2s]                                                                                                                                 
✔ |        29 | Utility functions for building args, etc. [0.1s]                                                                                                    
✔ |        39 | testing a composable workflow and running bbi [65.1s]

barrettk commented 2 years ago

Below is the output when running older versions (3.1.1 and below) of bbi. check_run_times() will technically report incorrect values if using those versions, so we will have to add a warning in those cases

> devtools::test()
ℹ Loading bbr
ℹ Testing bbr
✔ | F W S  OK | Context
✔ |        21 | bbr exec functions [0.5s]                                                                                                                           
✔ |        13 | checking if models are up to date [0.3s]                                                                                                            
✔ |         9 | Collapse columns to string representation [0.4s]                                                                                                    
✔ |        73 | Constructing config log from bbi_config.json [2.6s]                                                                                                 
✔ |        40 | Copying model objects [0.5s]                                                                                                                        
✔ |         6 | copy-model-helpers [0.4s]                                                                                                                           
✔ |        34 | cov-cor [0.6s]                                                                                                                                      
✔ |        35 | Extract model paths from based_on fields [1.5s]                                                                                                     
✔ |        49 | Build paths from model object [0.4s]                                                                                                                
✔ |         6 | model_diff() comparing models [0.3s]                                                                                                                
✔ |     3   7 | Test bbi summary on multiple models [0.3s]                                                                                                          
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Skip (test-model-summaries.R:41:5): model_summaries.list produces expected output [BBR-SUM-006]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1

Skip (test-model-summaries.R:56:5): model_summaries.bbi_run_log_df produces expected output [BBR-SUM-008]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1

Skip (test-model-summaries.R:63:5): as_summary_list.bbi_summary_log_df works [BBR-SUM-009]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ |     5  10 | Test bbi summary functions [1.0s]                                                                                                                   
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Skip (test-model-summary.R:12:5): model_summary.bbi_nonmem_model produces expected output [BBR-SUM-001]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1

Skip (test-model-summary.R:53:5): model_summary() works with custom .ext file [BBR-SUM-002]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1

Skip (test-model-summary.R:108:11): model_summary() works with no .ext file [BBR-SUM-003]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1

Skip (test-model-summary.R:108:11): model_summary() works with no .grd file [BBR-SUM-003]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1

Skip (test-model-summary.R:108:11): model_summary() works with no .shk file [BBR-SUM-003]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔ |        73 | Modify attributes of model object [0.4s]                                                                                                            
✔ |        19 | Testing function to create or read in model object                                                                                                  
✔ |        28 | nm-file [0.2s]                                                                                                                                      
✔ |        29 | nm-join [1.0s]                                                                                                                                      
✔ |        20 | nm-tables [0.2s]                                                                                                                                    
✔ |        30 | Test bbi batch parameter estimate functions [6.8s]                                                                                                  
✔ |        11 | Test param_estimates functions [0.7s]                                                                                                               
✔ |        43 | test parsing labels for parameter table [1.6s]                                                                                                      
✔ |        33 | testing print methods for bbi objects [4.7s]                                                                                                        
✔ |         5 | read_bbi_path() helper function                                                                                                                     
✔ |        50 | Reading NONMEM output files into R [0.7s]                                                                                                           
✔ |        41 | Constructing run log from model yaml [1.3s]                                                                                                         
✔ |        10 | submit_model(.dry_run=T)                                                                                                                            
✔ |        14 | submit_models(.dry_run=T) [0.3s]                                                                                                                    
✔ |        75 | Test creating summary logs [7.5s]                                                                                                                   
✔ |         9 | Comparing tags between models [0.2s]                                                                                                                
✔ |        23 | test_threads(.dry_run=T) [3.8s]                                                                                                                     
✔ |         7 | test-use-bbi [3.5s]                                                                                                                                 
✔ |        29 | Utility functions for building args, etc. [0.1s]                                                                                                    
✔ |     1  35 | testing a composable workflow and running bbi [61.6s]                                                                                               
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Skip (test-workflow-bbi.R:229:5): check_run_times() .return_times arg [BBR-CRT-003]
Reason: bbi version is 3.1.1. Test requires bbi version > 3.1.1
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

══ Results ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 104.1 s

── Skipped tests  ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
• bbi version is 3.1.1. Test requires bbi version > 3.1.1 (9)

[ FAIL 0 | WARN 0 | SKIP 9 | PASS 887 ]

barrettk commented 2 years ago

@kyleam For reference: https://github.com/metrumresearchgroup/bbr/pull/512

Also I suppressed certain warnings that would show up when using older versions of bbi in check_run_times(). I wanted the function to still "work" with the older versions, despite not being entirely accurate (as you'll see in the code).

I personally dont like putting that code in, but didnt see an alternative if we wanted any of it to work. Let me know what you think

barrettk commented 2 years ago

I'll need to take a closer look, but an initial round of comments.

A couple of general comments in addition to the inline ones:

thanks for checking the newer bbi version drone.yml with gh-512. After seeing the number of tests skipped, I'd vote to cherry pick that drone commit to this PR so that drone's running with v3.2.0-alpha.1. We can open another issue (assigned to the 1.4.0 milestone) with a reminder to bump this to v3.2.0 before the bbr release (and after the bbi release).

looking at the tests and given the linked issue (gh-509), I was surprised to not see a new test that explicitly checks that a set of times is summed to the expected value.

Ill just redo the drone changes in this PR (prefer not to cherry pick), but yeah that sounds good - I was somewhat unnerved by the number of tests we were skipping as well.

I dont think a test like that is necessary, as thats just testing the functionality of sum(). If it was more complicated than that I certainly would have. When I initially wrote tests I received some push back on testing for specific values (was told dimensions were fine, since value testing was really a function of model_summary - which makes sense). I can add a test if need be, but IMO it seems kind of pointless unless we tested for edge cases like "NA" or some odd behavior.

kyleam commented 2 years ago

I dont think a test like that is necessary, as thats just testing the functionality of sum().

It's not the specific value that I'm interested in. It's some sort of test that actually distinguishes the change in behavior (i.e., aggregating rather than reporting a single value for one run). Perhaps that exists already and I'm missing it, but if not it would be good to test something that actually serves as a regression test for the "use all values" behavior.

kyleam commented 2 years ago

(prefer not to cherry pick),

Er, as long as you get the changes there, it's fine (and indistinguishable as an end result), but that's an odd preference :)

barrettk commented 2 years ago

hmm not sure why drone is failing. Passing on my end, but will look into it:

> devtools::test()
ℹ Loading bbr
ℹ Testing bbr
✔ | F W S  OK | Context
✔ |        21 | bbr exec functions [0.7s]                                                                                                                           
✔ |        13 | checking if models are up to date [0.5s]                                                                                                            
✔ |         9 | Collapse columns to string representation [0.7s]                                                                                                    
✔ |        73 | Constructing config log from bbi_config.json [2.2s]                                                                                                 
✔ |        40 | Copying model objects [0.6s]                                                                                                                        
✔ |         6 | copy-model-helpers [0.3s]                                                                                                                           
✔ |        34 | cov-cor [0.6s]                                                                                                                                      
✔ |        35 | Extract model paths from based_on fields [1.3s]                                                                                                     
✔ |        49 | Build paths from model object [0.3s]                                                                                                                
✔ |         6 | model_diff() comparing models [0.4s]                                                                                                                
✔ |        19 | Test bbi summary on multiple models [0.9s]                                                                                                          
✔ |       103 | Test bbi summary functions [1.4s]                                                                                                                   
✔ |        73 | Modify attributes of model object [0.4s]                                                                                                            
✔ |        19 | Testing function to create or read in model object [0.1s]                                                                                           
✔ |        28 | nm-file [0.2s]                                                                                                                                      
✔ |        29 | nm-join [0.9s]                                                                                                                                      
✔ |        20 | nm-tables [0.3s]                                                                                                                                    
✔ |        30 | Test bbi batch parameter estimate functions [6.2s]                                                                                                  
✔ |        11 | Test param_estimates functions [0.6s]                                                                                                               
✔ |        43 | test parsing labels for parameter table [1.6s]                                                                                                      
✔ |        33 | testing print methods for bbi objects [4.4s]                                                                                                        
✔ |         5 | read_bbi_path() helper function                                                                                                                     
✔ |        50 | Reading NONMEM output files into R [0.7s]                                                                                                           
✔ |        41 | Constructing run log from model yaml [1.5s]                                                                                                         
✔ |        10 | submit_model(.dry_run=T)                                                                                                                            
✔ |        14 | submit_models(.dry_run=T) [0.3s]                                                                                                                    
✔ |        75 | Test creating summary logs [7.9s]                                                                                                                   
✔ |         9 | Comparing tags between models [0.3s]                                                                                                                
✔ |        23 | test_threads(.dry_run=T) [4.1s]                                                                                                                     
✔ |         7 | test-use-bbi [3.1s]                                                                                                                                 
✔ |        29 | Utility functions for building args, etc. [0.1s]                                                                                                    
✔ |        39 | testing a composable workflow and running bbi [61.5s]                                                                                               

══ Results ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 104.4 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 996 ]

EDIT: ahh, accidentally put the new test outside of the options

metrumresearchgroup / bbr

Fix check_run_times and model_summary tests #511