Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a set of helper functions and supporting documentation to facilitate model selection and reporting. Key changes include the addition of a vignette outlining reporting helpers, new functions (getMinAICFit, isBoundaryFit, listModelsTested) with associated tests and documentation, and updates to the package namespace and DESCRIPTION.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/reporting-helpers.Rmd | Added a vignette demonstrating reporting helper functions |
| vignettes/.gitignore | Updated ignore patterns for vignette-generated files |
| tests/testthat/test-AICHelpers.R | Added tests verifying behavior of model selection helper functions |
| man/*.Rd | Added documentation files for getMinAICFit, isBoundaryFit, and listModelsTested |
| R/AICHelpers.R | Introduced helper functions for model selection and reporting |
| NAMESPACE & DESCRIPTION | Exported new functions and updated package metadata |
R/AICHelpers.R
Outdated
| args <- list(...) | ||
| fitList <- list() | ||
| for (currentArg in args) { | ||
| if (is.list(fitList) && inherits(try(AIC(currentArg), silent = TRUE), "try-error")) { |
There was a problem hiding this comment.
The condition incorrectly checks 'is.list(fitList)' instead of 'is.list(currentArg)', which may lead to improper expansion of list arguments. Consider replacing 'is.list(fitList)' with 'is.list(currentArg)' to correctly handle nested lists of fits.
| if (is.list(fitList) && inherits(try(AIC(currentArg), silent = TRUE), "try-error")) { | |
| if (is.list(currentArg) && inherits(try(AIC(currentArg), silent = TRUE), "try-error")) { |
There was a problem hiding this comment.
is.list(fitList) is always true
| @@ -0,0 +1,2 @@ | |||
| *.html | |||
| *.R | |||
There was a problem hiding this comment.
Double-check if ignoring '*.R' files in the vignettes directory is intentional, as it may inadvertently exclude necessary vignette source files needed during documentation builds. If this is by design, consider adding an explanatory comment to clarify the rationale.
There was a problem hiding this comment.
This is part of the default usethis::use_vignette() setup.
|
Again, other than the issue reported by copilot, this looks good to me and may be helpful to others. I'm unsure why the checks fail, though. |
I found functions like these very helpful for my day-to-day reporting needs, so I thought I'd make them generally available.