Feature: add_predictor#31
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reintroduces the db$add_predictor() convenience interface to write predictor metadata (pred_meta_t) and predictor values (pred_data_t) in one step, alongside an environment upgrade (R 4.6 + roxygen2 8.0.0) that regenerates documentation and workflows.
Changes:
- Add
add_predictor()(andevoland_db$add_predictor()) to upsert predictor metadata + data using the activeid_run. - Tighten/coerce
pred_meta_ttypes (incl.sourcesnormalization) and update tinytests accordingly. - Update CI workflows and regenerate Rd files due to roxygen changes.
Reviewed changes
Copilot reviewed 13 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rproject.toml | Bumps declared R version for the dev environment. |
| R/util_download.R | Clarifies that df_in can come from predictor/intervention metadata tables. |
| R/trans_models_t.R | Minor formatting/roxygen adjustments (comma cleanup, doc tweak). |
| R/pred_meta_t.R | Strengthens coercion for pred_meta_t fields and normalizes sources. |
| R/pred_data_t.R | Adds add_predictor() and adjusts printing/fill behavior for predictor columns. |
| R/evoland_db.R | Exposes add_predictor() as an R6 method on evoland_db. |
| R/evoland_db_views.R | Roxygen alias formatting cleanup. |
| man/util_terra.Rd | Roxygen-regenerated link target update for terra::SpatRaster. |
| man/util_download.Rd | Mirrors util_download.R doc change in generated Rd. |
| man/pred_meta_t.Rd | Mirrors pred_meta_t doc wording update in generated Rd. |
| man/pred_data_t.Rd | Documents newly added add_predictor() alias/usage. |
| man/parquet_db.Rd | Large roxygen formatting regeneration (method anchors/sections). |
| man/LearnerClassifGrrf.Rd | Roxygen formatting + link target regeneration. |
| man/evoland_db.Rd | Adds add_predictor() method to generated class docs + roxygen regeneration. |
| man/coords_t.Rd | Roxygen-regenerated link target update for terra::SpatExtent. |
| inst/tinytest/test_pred_meta_t.R | Extends pred_meta_t tests for missing sources behavior. |
| inst/tinytest/test_integ_trans_models_t.R | Updates plot-data assertions to match regenerated outputs. |
| inst/tinytest/test_db_evoland.R | Adds integration coverage for db$add_predictor() behavior. |
| DESCRIPTION | Updates roxygen config metadata to roxygen2 8.0.0. |
| .github/workflows/R-CMD-check.yaml | Adjusts matrix + cache paths; updates action versions (needs fixes). |
| .github/workflows/R-CMD-check-light.yml | Updates checkout action version (needs fixes). |
Files not reviewed (8)
- man/LearnerClassifGrrf.Rd: Language not supported
- man/coords_t.Rd: Language not supported
- man/evoland_db.Rd: Language not supported
- man/parquet_db.Rd: Language not supported
- man/pred_data_t.Rd: Language not supported
- man/pred_meta_t.Rd: Language not supported
- man/util_download.Rd: Language not supported
- man/util_terra.Rd: Language not supported
Comments suppressed due to low confidence (1)
R/pred_data_t.R:235
- In
set_pred_coltypes(),fill_valueis parsed viatype.convert(), but the assignment usesmeta_row$fill_value(a character, becausepred_meta_t$fill_valueis cast tochar). This can coerce numeric predictor columns to character when filling NAs, and it ignores the parsedfill_valuevalue entirely. Use the parsedfill_valuefor assignment (and handle factors explicitly if you need level-safe filling).
# if col is factor, fill_value being a character is safe
# dt set() can add a new level if it's not already present
fill_value <- meta_row$fill_value |> type.convert(as.is = TRUE)
if (!is.na(fill_value)) {
data.table::set(
result,
i = which(is.na(result[[col]])),
j = col,
value = meta_row$fill_value
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reintroduces the
db$add_predictorinterface that simultaneously writes topred_meta_tandpred_data_t. Incidentally, the dev env was upgraded to R 4.6 and roxygen 8.0.0 introduced, which makes it look like this PR is huge, but actually isn't.