tidyclust integration notes

[DavisVaughan]

Rather than making the most minimal change possible, I think it will be better for the long term code health of workflows to add tidyclust as a new model "action" (this is what actions were intended for anyways). This allows you:

• To isolate the tidyclust changes (for the most part) in fit-action-model-tidyclust.R
• To add a new add_model_tidyclust() API that is simpler and separate from add_model() (i.e. no formula argument for model formulas)
• To use check_conflicts() to ensure that the user can't add both a tidyclust model and a parsnip model to the workflow
  • We would add a method for both action_model and action_model_tidyclust types

Rather than Importing or Suggesting tidyclust, since it is so heavy I think we should just use something like dplyr:::import_vctrs() (but not in .onLoad() because we dont know if tidyclust will be installed) to access functions from tidyclust that we need to call. Since we own both packages, I think that this is okay.

[simonpcouch]

I’ll go ahead and revert the celery branch to where it had been yesterday so that dev tidyclust can continue to rely on that branch for now. :)

[simonpcouch]

Have spent some time this afternoon speccing out an implementation here. After thinking through this a bit more, I’m no longer sure that the add_model_tidyclust approach is more principled than the more minimal approach like that in the celery branch right now—generally, I feel like it’s not disanalogous to think of tidyclust like other parsnip extension packages in this case, even though it makes use of a different class and model environment.

Re: the first three bullets:

• This is partially true, though 1) much of the code in that new file would be near-duplicate of fit-action-model.R and 2) much of the model action infrastructure that the new model_tidyclust action would make use of still assumes a parsnip model specification. The changes needed there definitely aren’t extensive, but still put us in a place where we run code and generate prompts conditionally on object classes (as in pull_workflow_spec_encoding_tbl).
• add_model_tidyclust uses a different naming pattern than add_model, which ought to be add_model_parsnip if the two were to use the same naming pattern. This would still be the case if we went for a more general add_model_unsupervised() (and add_model_supervised()). As for formula, we can either 1) not allow formula to be passed for cluster_specs or 2) make the *_model functions generic and not include formula in the cluster_spec.
• check_conflicts is indeed a helpful tool for maintaining the two actions alongside each other, though we only need it if we differentiate the two as separate actions.

I would propose more understated support for tidyclust:

• Make minimal fixes, like those in celery currently, that avoid using the tidyclust namespace specifically and refer only to an unsupervised_spec class, as in add more general class to cluster_spec objects tidyclust#76. We can avoid tidyclust:: with the dplyr:::import_vctrs() approach, like you mentioned.
• Audit mentions of parsnip and its namespace in messages/warnings/errors, conditionally noting either the current parsnip reference or a more general reference for other models.
• Mention support for unsupervised models in a few places in docs. I feel okay with much of the docs calling out parsnip specifically, as this is already the case even though workflows also supports implementations from parsnip extension packages.

[DavisVaughan]

Some thoughts about how to factor new_action_model() to handle unsupervised specs

new_action_model <- function(spec, formula, ...) {
  if (is_model_spec(spec)) {
    new_action_model_spec(spec, formula)
  } else if (is_unsupervised_spec(spec)) {
    if (!is.null(formula)) {
      abort("cant use formulas here")
    }
    new_action_unsupervised_spec(spec)
  } else {
    abort("unknown spec")
  }
}

[simonpcouch]

Addressed in #184!

[github-actions]

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.