Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename estimators and package #18

Merged
merged 2 commits into from
May 11, 2023
Merged

Conversation

grovduck
Copy link
Member

This PR would close #6 by renaming all current estimators to specify that they are KnnRegressor estimators. It also changing the package name to scikit-learn-knn-regression, although we haven't yet renamed this in Github, but will before we merge this change. The imported module name also changes from sklearn_knn to sknnr.

To make the change locally, I did these steps:

  1. renamed all instances of scikit-learn-knn to scikit-learn-knn-regression in all files
  2. renamed all instances of sklearn_knn to sknnr in all files
  3. renamed estimators to be suffixed with KnnRegressor (Raw, Euclidean, Mahalanobis) or Regressor (GNN, MSN)
  4. changed directory name src\sklearn_knn to src\sknnr
  5. hatch env prune to get rid of scikit-learn-knn and test environments
  6. hatch shell to add scikit-learn-knn-regression environment
  7. hatch run test:all to add test environment

(The only hiccup I had was that the old scikit-learn-knn hatch environments were not deleted, but I can do that manually).

- Change estimator names to reflect that they are KnnRegressors
- Change package name to "scikit-learn-knn-regression"
- Change imported module name to "sknnr"
@grovduck
Copy link
Member Author

@aazuspan , a couple of possible changes to consider:

  1. Do we want e.g. EuclideanKnnRegressor or EuclideanKNNRegressor? I'm OK either way.
  2. Doing a bit of browsing on PyPI, it looks like both scikit-learn and sklearn prefixes are used for custom estimators. scikit-learn is more explicit, sklearn is shorter for an already long package name.

Copy link
Contributor

@aazuspan aazuspan left a comment

Choose a reason for hiding this comment

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

Looks great overall!

Do we want e.g. EuclideanKnnRegressor or EuclideanKNNRegressor? I'm OK either way.

I lean towards KNN just for consistency with GNN and MSN, but not a strong opinion.

Doing a bit of browsing on PyPI, it looks like both scikit-learn and sklearn prefixes are used for custom estimators. scikit-learn is more explicit, sklearn is shorter for an already long package name.

Good points. I guess if I had to choose I would go with the more explicit scikit-learn, but if you have any preference I'd happily defer to that.

README.md Show resolved Hide resolved
@grovduck
Copy link
Member Author

I lean towards KNN just for consistency with GNN and MSN

I agree. Changed to KNN in this commit.

I would go with the more explicit scikit-learn

I don't have a strong opinion, but agree that explicit is likely better.

@grovduck grovduck requested a review from aazuspan May 11, 2023 17:29
Copy link
Contributor

@aazuspan aazuspan left a comment

Choose a reason for hiding this comment

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

Perfect, looks good to merge!

@grovduck grovduck merged commit a0890b0 into fb_add_estimators May 11, 2023
@grovduck grovduck deleted the rename_estimators branch May 11, 2023 18:11
@grovduck
Copy link
Member Author

Cool, I've renamed the repository to scikit-learn-knn-regression on Github and run this on my local repo:

git remote set-url origin git@github.com:lemma-osu/scikit-learn-knn-regression.git

@aazuspan
Copy link
Contributor

Awesome, thanks!

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.

Finalize naming conventions for estimators and package
2 participants