Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a progress callback argument to the train() function to support non-interactive progress tracking, adds a new generic get_varimp for retrieving variable importance, and updates several S7 class definitions and validation helpers. The feedback highlights two key improvements: first, using length(outer_resampler@resamples) instead of outer_resampler@config@n to robustly determine the number of outer folds; second, reverting the use of let() to := in R/draw_box.R to maintain backward compatibility with older versions of data.table since no minimum version is specified in the package description.
There was a problem hiding this comment.
Pull request overview
This PR appears to improve compatibility with non-interactive callers (notably rtemis.server) by adding an explicit training progress callback, tightening/standardizing S7 class metadata, and introducing a small exported API for variable-importance retrieval.
Changes:
- Add an optional
progress(stage, current, total, message)callback totrain()and wire it into the outer resampling loop. - Introduce and export a new
get_varimp()S7 generic with methods forSupervised/SupervisedRes. - Standardize multiple S7
new_class()definitions by settingpackage = "rtemis"and adjust dependency/check helpers (addrtemis.core).
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| R/train.R | Adds progress callback argument and emits fold-boundary progress events during outer resampling. |
| R/draw_box.R | Attempts to avoid mutating dt before melt() by switching from := to let(). |
| R/decomp.R | Header comment tweak. |
| R/16_S7utils.R | Header comment tweak. |
| R/14_SuperConfig.R | Adds package = "rtemis" to S7 classes; modifies roxygen block near class defs. |
| R/13_Themes.R | Adds package = "rtemis" to S7 class; modifies roxygen block near class def. |
| R/12_Decomposition.R | Adds package = "rtemis" to S7 class; modifies roxygen block near class def. |
| R/11_DecompositionConfig.R | Adds package = "rtemis"; swaps numeric checks to rtemis.core::check_numeric(). |
| R/10_Clustering.R | Adds package = "rtemis"; modifies roxygen block near class def. |
| R/09_ClusteringConfig.R | Adds package = "rtemis"; swaps numeric checks to rtemis.core::check_numeric(). |
| R/08_MassUni.R | Adds package = "rtemis" and adjusts class documentation tags. |
| R/07_Supervised.R | Adds package = "rtemis" to several classes and adds get_varimp methods. |
| R/06_Tuner.R | Adds package = "rtemis"; modifies roxygen block near class defs. |
| R/05_Resampler.R | Adds package = "rtemis"; modifies roxygen block near class defs. |
| R/04_Preprocessor.R | Adds package = "rtemis"; modifies roxygen block near class defs. |
| R/03_Metrics.R | Adds package = "rtemis"; modifies roxygen block near class def. |
| R/02_Hyperparameters.R | Adds package = "rtemis"; swaps numeric checks to rtemis.core::check_numeric(). |
| R/01_ExecutionConfig.R | Adds package = "rtemis"; modifies roxygen block near class def. |
| R/00_S7init.R | Defines/export get_varimp S7 generic and roxygen docs/examples. |
| NAMESPACE | Exports get_varimp. |
| man/train.Rd | Documents new progress argument. |
| man/get_varimp.Rd | Adds generated documentation for get_varimp. |
| Makefile | Changes log timestamp formatting. |
| DESCRIPTION | Bumps version/date; adds rtemis.core to Imports; removes plumber from Suggests. |
| .Rbuildignore | Removes one ignore entry; adds ignore pattern for PDFs. |
Files not reviewed (2)
- man/get_varimp.Rd: Language not supported
- man/train.Rd: Language not supported
Comments suppressed due to low confidence (1)
R/draw_box.R:972
dt[, let(ID = .I)]does not preserve the existing columns ofdt, sodata.table::melt()will likely fail becauseid.varsincludes columns liketimeperiod/group/hovertextthat are no longer present. To avoid mutatingdtwhile keeping all columns, use something likecopy(dt)[, ID := .I](or addIDthen remove it after melting).
# appease R CMD check ?rm timeperiod <- NULL
ID <- timeperiod <- NULL
dtlong <- data.table::melt(
dt[, let(ID = .I)],
id.vars = c(
"ID",
"timeperiod",
mgetnames(dt, "group", "hovertext")
)
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.