Skip to content

Factor key - #304

Open
topepo wants to merge 5 commits into
mainfrom
factor_key
Open

Factor key#304
topepo wants to merge 5 commits into
mainfrom
factor_key

Conversation

@topepo

@topepo topepo commented Jun 8, 2026

Copy link
Copy Markdown
Member

Adds factor_key() to create mappings between original factor variables and their binary indicator columns in model matrices. This function helps understand how factors are encoded with different contrast methods, including support for interactions and nested effects.

topepo and others added 4 commits April 12, 2026 15:56
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@topepo
topepo requested a review from hfrick June 8, 2026 15:54

@hfrick hfrick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you intend for this to get used? I'm lacking a little bit of context here, so I reviewed from a hardhat perspective.

The biggest conceptual feedback: From a hardhat perspective, factor_key.terms() should be an internal function and the exported methods are for the three types of blueprints.

This would also streamline the interface with regard to the data argument: For recipes and xy blueprints, we don't use the data argument at all. For the formula blueprint, we pass it to the terms function. However, if we pass on user input like that, data could differ from the data used to create the blueprint. Since we don't need actual data values but rather the factor levels, I would suggest that we reconstruct a mocked data frame based on the ptypes in the blueprint (which includes the factor levels).

Comment thread R/factor-key.R
# Also wrap in tryCatch to handle single-level factors gracefully
mm <- tryCatch(
{
with_na_pass(model.matrix(x, data))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation above (line 38) says we're using model_matrix(), so I suggest we align those two.

Comment thread R/factor-key.R

# Generate the model matrix to get actual column names
# Use with_na_pass to handle missing values properly
# Also wrap in tryCatch to handle single-level factors gracefully

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we say above

#' The function uses the same model matrix generation as [model_matrix()],
#' ensuring consistency with how your models will actually be fit.

I would expect us to error here, too.

If you need us to return the empty tibble, I would try to avoid grepping on the text of the error message since that's one that might get translated if someone's using R in a non-English locale. We could look for single-level factors via stats::.getXlevels(terms, data) and also warn alongside the empty tibble.

Comment thread R/factor-key.R
Comment on lines +117 to +119
factor_key <- function(x, ...) {
UseMethod("factor_key")
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the standard .default() method to error with the input type? hardhat has that in other places as well.

Comment thread R/factor-key.R
Comment on lines +187 to +190
# Get the assign attribute which maps columns to term indices
assign_attr <- attr(mm, "assign")
mm_colnames <- colnames(mm)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this down below the two blocks on factors_matrix since we don't use it immediately?

Comment thread R/factor-key.R
#' @inheritParams validate_column_names
#'
#' @rdname factor_key
#' @export

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to export this or rather treat it has an internal function and the exported methods are for the blueprints? Having worked through the rest, I now think we do indeed want this to be internal.

Comment thread NEWS.md
@@ -1,5 +1,7 @@
# hardhat (development version)

* Added `factor_key()` to create mappings between original factor variables and their binary indicator columns in model matrices. This function helps understand how factors are encoded with different contrast methods, including support for interactions and nested effects.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Added `factor_key()` to create mappings between original factor variables and their binary indicator columns in model matrices. This function helps understand how factors are encoded with different contrast methods, including support for interactions and nested effects.
* Added `factor_key()` to create mappings between original factor variables and their binary indicator columns in model matrices. This function helps understand how factors are encoded with different contrast methods, including support for interactions and nested effects (#304).

Comment on lines +3 to +6
library(modeldata)

# Single factor with default treatment contrasts
data(penguins)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
library(modeldata)
# Single factor with default treatment contrasts
data(penguins)
# Single factor with default treatment contrasts
data(penguins, package = "modeldata", envir = rlang::current_env())

The library() has effects outside of just the execution environment for the testthat() call. I like loading just the data, into just that environment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

library(testthat)

test_that("this is expected", {
  # `boost_tree()` is only available after loading parsnip
  expect_error(boost_tree())
  library(parsnip)
  expect_silent(boost_tree())
})
#> Test passed with 2 successes 🎊.

test_that("but loading parsnip persists beyond the scope of the first test", {
  # boost_tree() is still available, even though parsnip is not loaded again in this test
  # thus the expect_error() fails
  expect_error(boost_tree())
})
#> ── Failure: but loading parsnip persists beyond the scope of the first test ────
#> Expected `boost_tree()` to throw a error.
#> Error:
#> ! Test failed with 1 failure and 0 successes.

Created on 2026-06-10 with reprex v2.1.1

Comment thread R/factor-key.R
Comment on lines +199 to +245
# Get term labels
term_labels <- attr(x, "term.labels")

# Build mapping data
mapping_list <- list()

for (i in seq_along(mm_colnames)) {
col_name <- mm_colnames[i]
term_index <- assign_attr[i]

# Skip intercept (term_index == 0)
if (term_index == 0) {
next
}

# Get the term label
term_label <- term_labels[term_index]

# Find which variables contribute to this term
contributing_vars <- rownames(factors_matrix)[
factors_matrix[, term_index] > 0
]

# Filter to only factor variables
factor_contributors <- intersect(contributing_vars, factor_vars)

# If this column has factor contributors, add to mapping
if (length(factor_contributors) > 0) {
for (factor_var in factor_contributors) {
mapping_list[[length(mapping_list) + 1]] <- data.frame(
source = factor_var,
derived = col_name,
stringsAsFactors = FALSE
)
}
}
}

# Combine all mappings into a single data frame
if (length(mapping_list) > 0) {
result <- do.call(rbind, mapping_list)
result <- tibble::as_tibble(result)
} else {
result <- tibble::tibble(source = character(), derived = character())
}

result

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Get term labels
term_labels <- attr(x, "term.labels")
# Build mapping data
mapping_list <- list()
for (i in seq_along(mm_colnames)) {
col_name <- mm_colnames[i]
term_index <- assign_attr[i]
# Skip intercept (term_index == 0)
if (term_index == 0) {
next
}
# Get the term label
term_label <- term_labels[term_index]
# Find which variables contribute to this term
contributing_vars <- rownames(factors_matrix)[
factors_matrix[, term_index] > 0
]
# Filter to only factor variables
factor_contributors <- intersect(contributing_vars, factor_vars)
# If this column has factor contributors, add to mapping
if (length(factor_contributors) > 0) {
for (factor_var in factor_contributors) {
mapping_list[[length(mapping_list) + 1]] <- data.frame(
source = factor_var,
derived = col_name,
stringsAsFactors = FALSE
)
}
}
}
# Combine all mappings into a single data frame
if (length(mapping_list) > 0) {
result <- do.call(rbind, mapping_list)
result <- tibble::as_tibble(result)
} else {
result <- tibble::tibble(source = character(), derived = character())
}
result
# For each model matrix column, find the factor variables contributing to it
# (the intercept maps to no factors, so it contributes an empty character)
contributors <- map(assign_attr, function(term_index) {
if (term_index == 0) {
return(character())
}
contributing_vars <- rownames(factors_matrix)[
factors_matrix[, term_index] > 0
]
intersect(contributing_vars, factor_vars)
})
tibble::tibble(
source = unlist(contributors),
derived = rep(mm_colnames, lengths(contributors))
)

This is Claude's suggestion when I asked it to use hardhat's pattern of "assemble columns, build once" which also avoids growing that list. Tests passed.

Comment on lines +204 to +207
# Skip if contr_one_hot is not available
if (!exists("contr_one_hot", mode = "function")) {
skip("contr_one_hot not available")
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hardhat exports contr_one_hot() so we don't need this block

Comment thread R/factor-key.R
#' - [stats::contrasts()] for setting contrast methods
#'
#' @export
factor_key <- function(x, ...) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to get_factor_key(), like other exported get_*() functions in hardhat, e.g., get_levels(), get_outcome_levels()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants