search

metrumresearchgroup / bbr

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

Filtering Parameter in run_log and summary_log #484

Closed rymarinelli closed 2 years ago

rymarinelli commented 2 years ago

Closes #477

rymarinelli commented 2 years ago

For test-run-log.R, [BBR-RNLG-004] will fail if non-numeric run ids are used. It might be a good idea to revise this using a deep copy for a test to compare ids instead.

kyleam commented 2 years ago

I played around with summary_log a bit using the example project, and it worked as advertised. However, I have a question about the direction of filtering. Based on what I'm seeing in the example project, the goal is usually to filter to some specified subset of models. The associated issue for this PR (#477) suggests that this same behavior: " pass a vector of run ID's (i.e. the values that show up in the run column in the resulting tibble) to any of the *_log() functions and have the resulting tibble filtered to only runs in that vector".

But the approach of this PR is to filter out the values. I'm not spotting any discussion about this switch in the approach. @rymarinelli Did you and @seth127 decide to filtering out was preferred for some reason?

seth127 commented 2 years ago

Sorry, that was a misclick on the close-and-reopen.

Yes, the including based on the vector is what we want. I was using the term “filter” based on dplyr::filter() which includes based on the filter criteria. I once saw a talk from Hadley where he noted that “filter” may not have been the best name choice for this (because of the ambiguity we had here) but most R folks think of it this way at this point.

All that to say, yes this looks good to me. I think .include is a fine arg name and unambiguous, which is good.