Skip to content

Fix entry point name for Logistic Regression (LogisticRegressor is misleading) #139

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 14, 2018
Merged

Fix entry point name for Logistic Regression (LogisticRegressor is misleading) #139

merged 2 commits into from
May 14, 2018

Conversation

GalOshri
Copy link
Contributor

This PR fixes #114.

We currently have LogisticRegressor and BinaryLogisticRegressor entry points, which are misleading (make logistic regression seem like it is used for regression) and don't match the other learners. These are renamed to LogisticRegressionClassifier and LogisticRegressionBinaryClassifier respectively.

I have not tried modifying entry points before, so please let me know if something is missing. Here is what I did:

  1. Update the entry point names in the learners (binary and multiclass LR)
  2. Regenerate CSharpAPI.cs
  3. Update TestCSharpAPI.cs and TestEntryPoints.cs
  4. Update core_ep-list.tsv and core_manifest.json to reflect the new names
  5. Update remaining references to the old names

This is a breaking change to the Logistic Regression C# APIs.

@GalOshri GalOshri requested review from codemzs, yaeldekel and glebuk May 13, 2018 19:04
TomFinley
TomFinley approved these changes May 13, 2018
View reviewed changes
Copy link
Contributor

@TomFinley TomFinley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

:shipit:

Thanks much Gal... this also jibes with @yaeldekel 's PR 113 which is nice.

codemzs
codemzs approved these changes May 14, 2018
View reviewed changes
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

Thanks, @GalOshri! LGTM.

@shauheen shauheen merged commit 1b08ae4 into dotnet:master May 14, 2018
@codemzs codemzs mentioned this pull request Jun 8, 2018
Closed
eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018
@GalOshri @eerhardt
Fix entry point name for Logistic Regression (LogisticRegressor is mi…
3970b82 
…sleading) (dotnet#139)

* Fix name for Logistic Regression
@GalOshri GalOshri deleted the logistic_regressor_name_fix branch August 31, 2018 18:20
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@codemzs codemzs codemzs approved these changes

@yaeldekel yaeldekel Awaiting requested review from yaeldekel

@glebuk glebuk Awaiting requested review from glebuk

+1 more reviewer

@TomFinley TomFinley TomFinley approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Entry point name for logistic regression is misleading
4 participants
@GalOshri @codemzs @TomFinley @shauheen