Naming and where to use the method

[topepo]

First, I'm really happy that you implemented this model. I like it a lot, and it has felt very overlooked.

Second, I need to apologize: I should have given you feedback on the package early (since you filed an issue). Our pre-conference work got in the way and I had not kept up with my GH notifications.

I wanted to make some suggestions about altering the APIs that interface with tidymodels a bit (mostly around naming). It's already of CRAN but, if you agree with the proposals, I don't think it would be hard to make changes.

I'd like to have either a better name for the parsnip function or move dann and sub_dann to be engines for an existing model type (either nearest_neighbor() or discrim_linear()). We like the names to be self-explanatory and we don't want to start a trend of prefixing parsnip models with tidy_. If you want a new parsnip model name, maybe discrim_linear_local or discrim_linear_nearest_neighbors?

Also, for technical reasons, it helps to have the overall parsnip model definition inside of parsnip. That enables the engine-specific documentation (which we have had a lot of positive feedback on). Not all models do this (like those in the modeltime package), but it makes things a lot easier overall.

Naming-wise, it would also help to have a better name for epsilon. We've always espoused not using jargon in our argument names and would like to be consistent. The original paper describes it as a "softening" parameter (and it is basically a ridge parameter like in Newton-Raphson) so maybe call it matrix_diagonal?

If you are interested in making the changes, you could deprecate the original tidy_* functions and make a new one (or add it to an existing model).

[gmcmacran]

No problem. Thanks for taking to time to review the code. I am open to anything that is consistent with the broader tidymodel's ecosystem.

For renaming epsilon, I agree. matrix_diagonal it is.

I can move model definition to parsnip.

For nearest_neighbor, the tidy_sub_dann function would add 5 additional arguments that don't apply to other engines. In addition, many of nearest_neighbor's arguments don't apply to sub_dann. This makes me think dann and sub_dann are different enough to justify a new function. My vote is adaptive_nearest_neighbor. Let me know if you agree or disagree.

[topepo]

Thanks!

No issues with a new model type. How about, to work with tab-complete, nearest_neighbor_adaptive()?

If it were to be in an existing model, we would set up some methods so that those extra parameters could be added as engine arguments and automatically tuned..

[gmcmacran]

nearest_neighbor_adaptive works for me.

Think this covers everything. I will get started on the name changes tomorrow night.

[gmcmacran]

@topepo I am a bit confused on how to handle dependency between tidydann and parsnip.

Under current code, the package tidydann depends on parnsip for development functions (set_dependency, set_model_arg, etc). Parsnip's nearest_neighbor_adaptive depends on tidydann to provide engines for a model.

If you could provide general context and an example of how this is handled for other models, it would be helpful.

[gmcmacran]

@topepo I think this covers all the suggestions. Let me know if you agree or not.

[gmcmacran]

@topepo Updates are on main. I believe this covers all of your suggestions. I would like approval to add the package to the model spec search before releasing to cran https://github.com/tidymodels/tidymodels.org/pull/41. Let me know if I need to change anything else. Otherwise, I see the main branch as ready for release.

[gmcmacran]

Closing the issue. Feel free to reopen if needed.

[gmcmacran]

Closing the issue. Feel free to reopen if needed.