-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Model tree diagram #673
Model tree diagram #673
Conversation
c0748e2
to
3626ef8
Compare
Can |
|
existing_sum_cols <- sum_cols[sum_cols %in% names(full_log)] | ||
sum_log <- sum_log %>% dplyr::select(-all_of(existing_sum_cols)) | ||
} | ||
full_log <- dplyr::left_join(full_log, sum_log, by = req_cols) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barrettk should n row
of full_log
always be the same before and after this left_join
? if so, can add a check for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andersone1 Shouldn't the join enforce that? If we join by req_cols
I cant think of any way the number of rows would change...unless you made the run_log
and then added a new model or deleted an old one before running model_tree()
, though that seems like an edge case (I think they would still be the same number of rows though). Can you think of any other way that could happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barrettk I can't really think of a way it could happen, but left_join
by does not enforce that.
- silence these warnings when creating a run_log or adding columns to it via one of the bbr::add_*() functions. - eliminates the need to suppress these warnings within model_tree() related functions - this warning doesnt provide any value and should be silenced in more places, but those can be done in a separate, more dedicated PR
… column to a run log.
- Defaults to an interactive HTML plot and optionally a static plot - Control tooltip settings and optionally append model_summary() output (defaults to including this) - Tags and other elements are extracted and nicely formatted. - Control how to color coat the nodes. Uses bbr's logo colors and colors by the model run by default, though one could see this being useful for quickly identifying which models are starred, have heuristics, etc. - supports multiple based_on flags per model - supports multiple origin models (models with no based_on flag) - supports broken links (when based_on points to a given model, but the model cannot be found) - Uses S3 methods, and can take in either a run log or a modeling directory
- needed for coloring the tree nodes. Should already be installed with `ggplot2`, which has been a `bbr` suggested package for a while. If we remove `ggplot2` as a suggested package, perhaps we will want to revisit this as well.
- add zoomable argument, allowing you to pan and zoom with the interactive plot - this can be valuable in the event any tooltips are cutoff. Defaults to FALSE because the zoom feature can be annoying if you're not expecting it.
- digits argument for rounding numeric values - works with newly defined columns via mutate calls - Can work with multiple working directories (i.e. if you row binded mutliple run_logs, it can handle the multiple origins) - color_by bug fixes. Can also pass `NULL` as to not color by any column. - more checks for helper functions - better handling for static plots. Instead of plotting it within the function, we now return the data and implement a print method. I referenced what ggplot2 did for this handling and I think its notably more portable now. - fill out remaining tests
- This was somewhat of a large refactor. Before this refactor, nested models could not be properly linked. Nested models have `based_on` values of `../2` or something similar, which broke the ability to map it to its parent model - To address this, we first have to determine which models are stored in a nested fashion by seeing if any of the model directories are subdirectories of another, and extracting the relative file path if so. After doing this, we have to overwrite the `based_on` attribute to just be the model id, and make the `run` column include the nested directory. This will cause the subdirectory to show in the tooltip as part of the model run, and will allow the models to be linked. - Given the complexity of some of the logic here, I opted to write a combined test that simulates a variety of scenarios all happening at once to ensure there are no interactions between them (broken links, recursive model directory, additional based on references, etc.).
- This was only caught with local R CMD Check. The `R_CHECK_LENGTH_1_CONDITION` environment variable controls this, though apparently this was ditched in R 4.2, in favor of making this concern an error instead of a warning. Not sure if either of those things are relevant as to why it would pass on drone and not locally, but just documenting what I found on the subject.
- In making the vignette I found a bug when coloring by a logical column. If all values were `TRUE`, all nodes would be colored white. Logical columns should be colored the same regardless of the unique values, so a refactor was necessary to explicitly set FALSE values to white and TRUE values to red. - Additional tests were added to ensure we catch this in the future.
- also improve one of the examples
- dont call model_summary() when possible - skip tests if required bbi version not met and model_summary() is required
- model type is now displayed - bootstrap runs will show up in the model tree - `--simulation attached--` will appear if an attached simulation is found and `add_summary` is TRUE
- now includes bootstrap and simulation examples
- we could add a step to install this in the test suite, but we havent historically done that, and this aligns more with the current methods of skipping tests if suggested packages aren't installed
- wasnt meant to display in vignette
9a86b0c
to
95016ff
Compare
- skip_if_not_drone_or_metworx --> skip_if_not_ci_or_metworx
Most of the check failures are straightforward. I can help take a look at the oldest job failure: https://github.com/metrumresearchgroup/bbr/actions/runs/9701317535/job/26774698747
|
1.3.0 isn't on MPN 2023-01-25 (it wasn't released until Nov 2023). @barrettk is this constraint just copied from ggplot2? Or are we using scales directly in a way that requires 1.3.0 or later? If the former, I think we can drop the constraint, and hopefully that will resolve the pak failure. |
@kyleam It wasn't copied from See |
I see. Then the options I can think of are
I think you're going with the first option (no objections from me). |
- move to print.R for consistency
Highlighting this note, since it won't fail the build: https://github.com/metrumresearchgroup/bbr/actions/runs/9702122713/job/26777261543
|
@kyleam can you take a look at the CI before and after I added Edit: the B) I reported was because I accidentally hit 'enter' and didnt realize when I went to commit that change. A) is the only uncertainty |
- must have accidentally hit 'enter'
You don't need the custom step as long as it can be retrieved from the date-pinned RSPM URL. |
Would you recommend reverting this change then, or is there potentially a reason to keep it (some form of risk mitigation)? |
yes |
- From KyleM: You don't need the custom step as long as it can be retrieved from the date-pinned RSPM URL - Note: we could add scales 1.3.0 at some point instead (to run more tests in CI), but we have functions available now to skip these. mpn-latest will execute them too so its not much of a risk
- 0.1.7 is available from the date-pinned RSPM URL in (2023-01-25 cran snapshot). This version also included important bug fixes related to the new model_tree() function, so it makes sense to add before merging
Introduces new
model_tree()
function, allowing you to create a tree diagram for your modeling directorySome Example function calls
Example output
Logical values will be colored as white and red, and are referred to as 'border colors'. The more unique values are added, the more gradient colors between those two border colors are generated.
This example illustrates how
NA
values are handled. Here we color by description (which wouldn't make sense to do in practice). The two distinct descriptions are colored white and red, whereNA
values are colored grey.Previous PR for reference: #610