Skip to content

Conversation

brendad8
Copy link

Closes #5

@kbodwin

Note: unstated dependency dbscan:::predict_frNN()
- This function is found in db_clust.R and predict_helpers.R

@EmilHvitfeldt
Copy link
Member

Hello @brendad8 👋

This is a big amount of effort you have put into this, thank you!

Before I give a more thorough review, could you move the code away from {Rfast} to {philentropy} like it is being done in: https://github.com/tidymodels/tidyclust/pull/199/files

@brendad8
Copy link
Author

Hello Emil. Just took care of this in the last few commits. Let me know if you need anything from me or have any questions

@kbodwin
Copy link
Collaborator

kbodwin commented Jun 19, 2025

@EmilHvitfeldt - just a heads up, we have two more big PRs headed our way. I'm putting together some documentation about what was added and what are the bigger changes we might need to discuss, I share that as an Issue on here soon! :)

@EmilHvitfeldt
Copy link
Member

Thanks for the heads up! I'm gonna try to review what I have Friday, if not then certainly Monday/Tuesday

Copy link
Member

@EmilHvitfeldt EmilHvitfeldt left a comment

Choose a reason for hiding this comment

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

overall looking good.
In addition to the requested changes, we need to make sure the GHAs run clean, one thing we need is for you to add the newly added models to _pkgdown.yml

@EmilHvitfeldt
Copy link
Member

please also add NEWS bullets in accordance to https://style.tidyverse.org/news.html

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.

Engine: mclust()
3 participants