Adding dann

[gmcmacran]

I would like to add my modeling package to the tidymodels ecosystem. I am working on a major release that refactors the code base how tidymodel's hardhat suggests. Models will have a recipe method for training, and predict returns tidymodel's preferred prediction format. I am holding off on submitting to cran until I get it added to tidymodels.

Github link: https://github.com/gmcmacran/dann/tree/hardhat

My question are:
1: Are you guys open to the addition of two new models? dann and sub_dann.
2: Should I issue a PR myself?
3: What package should I issue it in? Should I make a PR in parsnip or create a new package similar to poissonreg?
4: Is there a blog post that serves as a good starting point for tidymodels development? Otherwise, I will use poissonreg as a reference.

[simonpcouch]

Thanks for the issue! dann looks like a neat package.

This sounds like a great idea for a parsnip extension package. parsnip's infrastructure allows for anyone to create their own package that integrates with parsnip (and thus the rest of the tidymodels ecosystem)! We have some documentation on how to create custom model specifications on the tidymodels website. We'd recommend creating a new package of your own—poissonreg would indeed be a great place to see how those specifications are situated in a package, and modeltime is an example of another community-contributed package.

Feel free to holler if you run into any issues while building the package or have a finished product you'd like us to feature on the tidymodels website.

[gmcmacran]

Excellent! I will take a look at the links and get started.

[gmcmacran]

My tidydann package defines an S3 method tunable. https://github.com/gmcmacran/tidydann/blob/cf7208a3a6894ffc4bcc0d4d09f2f1dc81947cb4/R/tidy_dann.R#L50-L112

With my package loaded, tune_grid fails. If I define the same exact function within the R script (outside of the package), tune_grid works as expected. https://github.com/gmcmacran/tidydann/blob/cf7208a3a6894ffc4bcc0d4d09f2f1dc81947cb4/R/question.R#L18-L37

Can you explain why this is happening and how to fix the problem?

For context, fit and predict work fine when hyperparameters are set to specific values by the user.

[hfrick]

I suspect that might be because tune_grid() also has to know about (and load) the package which contains the model and the engine, so tidydann itself. That should be declared as one of the dependencies for the engine, in addition to dann here:
https://github.com/gmcmacran/tidydann/blob/cf7208a3a6894ffc4bcc0d4d09f2f1dc81947cb4/R/zzz.R#L11

We have draft of a checklist of all the details to remember when adding new models and engines, in addition to the resources Simon already pointed you towards. It's at https://github.com/tidymodels/tidymodels/pull/97/files - that might be helpful as well. The parts that say "goes in parsnip" would be "goes in tidydann" in your case. We are happy for feedback on it, too, if there are still gaps!

[gmcmacran]

(Documenting to help the next person who has this challenge.)

I added the second set_dependency. https://github.com/gmcmacran/tidydann/blob/4213cbaaf586c6bef7f8d7ddb62a6dec8b4b3837/R/zzz.R#L12C12-L12C26, but this alone did not fix the issue.

I mimicked tidymodels/parsnip. The function tunable.tidy_dann became tunable_tidy_dann https://github.com/gmcmacran/tidydann/blob/4213cbaaf586c6bef7f8d7ddb62a6dec8b4b3837/R/tidy_dann.R#L104-L112 and then registered during .onLoad https://github.com/gmcmacran/tidydann/blob/4213cbaaf586c6bef7f8d7ddb62a6dec8b4b3837/R/zzz.R#L112.

This changes caused the CV to work as expected. https://github.com/gmcmacran/tidydann/blob/4213cbaaf586c6bef7f8d7ddb62a6dec8b4b3837/R/question.R#L16-L22

[hfrick]

Glad you got it working!

A note on registering an S3 method via vctrs::s3_register(): this is typically only necessary if you are registering a method for a class that's in a package you don't depend on. I don't see any dependencies in our current code but I assume that you're likely to add at least parsnip to that list. You can get around the S3 registration in .onLoad(), if you import the generic. The generic for tunable() sits in the {generics} package which is itself a dependency of parsnip so it wouldn't make your dependencies list much heavier. From the R packages book:

An add-on package that implements an S3 generic for a new class should list the generic-providing package in Imports, import the generic into its namespace, and export its S3 method.

[gmcmacran]

That section of the R packages book was the missing piece! I was able to renamed the function to tunable. and remove the vctrs::s3_register() in onLoad.

[gmcmacran]

In model specifications, how do I indicate my modesl cannot handle case weights? I cannot track down an example or documentation about changing this.

[simonpcouch]

You can do so by not adding weights to the protect entry for the model. e.g. do this:

parsnip/R/mlp_data.R

Line 65 in 907d216

protect = c("x", "y"),

instead of this:

parsnip/R/mars_data.R

Line 44 in 907d216

protect = c("formula", "data", "weights"),

Some background:

parsnip/R/case_weights.R

Lines 85 to 88 in 907d216

	# If weights are used, they are protected data arguments with the canonical
	# name 'weights' (although this may not be the model function's argument name).
	data_args <- model_info$value[[1]]$protect
	any(data_args == "weights")

When !case_weights_allowed(spec), you'll see:

parsnip/R/misc.R

Lines 513 to 516 in 907d216

	allowed <- case_weights_allowed(spec)
	if (!allowed) {
	rlang::abort("Case weights are not enabled by the underlying model implementation.")
	}

🙂

[gmcmacran]

Think I am missing something. In https://www.tidymodels.org/learn/develop/models/ under step 3, the protect argument does not contain weights and yet show_model_info("discrim_mixture") indicates the model can use case weights.

[simonpcouch]

Ah, that print method is a bit unclear in that case, huh. This might resolve your confusion:

library(tidymodels)

# custom model info:
set_new_model("discrim_mixture")
set_model_mode(model = "discrim_mixture", mode = "classification")
set_model_engine(
  "discrim_mixture", 
  mode = "classification", 
  eng = "mda"
)
set_dependency("discrim_mixture", eng = "mda", pkg = "mda")

show_model_info("discrim_mixture")
#> Information for `discrim_mixture`
#>  modes: unknown, classification 
#> 
#>  engines: 
#>    classification: mdaNA
#> 
#> ¹The model can use case weights.
#> 
#>  no registered arguments.
#> 
#>  no registered fit modules.
#> 
#>  no registered prediction modules.

show_model_info("linear_reg")
#> Information for `linear_reg`
#>  modes: unknown, regression 
#> 
#>  engines: 
#>    regression: brulee, glm¹, glmnet¹, keras, lm¹, spark¹, stan¹
#> 
#> ¹The model can use case weights.
#> 
#>  [truncated output for clarity]

^{Created on 2023-09-14 with reprex v2.0.2}

"¹The model can use case weights." means that only engines with the superscripted 1 can use case weights. In cases where some engines can support case weights, the notation is clear, but when there's no case weight support for any engine (and thus no superscripted 1 previouslt), that footnote seems like it's saying that every engine used with the model can use case weights. We ought to fix that—I'll open up a separate issue.

[gmcmacran]

Thanks. That clears everything up for me.

[gmcmacran]

I am finishing up tidydann. Thank you both for the help. The detailed answers y'all provided were very helpful. You guys can check out the nearly final version in the repo if y'all are interested. I plan on submitting dann v1.0.0 tomorrow and tidy in following few days.

dann v1.0.0: https://github.com/gmcmacran/dann (not on cran yet)
tidydann: https://github.com/gmcmacran/tidydann/tree/initial