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

Add linfa-linear package with ordinary least squares regression #20

Merged
merged 22 commits into from
Jul 20, 2020

Conversation

paulkoerbitz
Copy link
Collaborator

This PR adds the linfa-linear crate with an ordinary least squares regression model.

It is in spirit similar to PR 10 which looks like it has stalled. It also addresses the issues discussed in this PR and ads example code and tests.

Copy link
Member

@bytesnake bytesnake left a comment

Choose a reason for hiding this comment

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

hey, added two nitpicks, if you think this is ready we can merge it. I'm a bit ambigious about the error handling, but at the moment we are mostly collecting algorithms so a simple text approach is fine :D The gzip training data is really cool though

FitInterceptAndNormalize,
}

fn fit_intercept(options: Options) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

can you implement this as a member of Options?

}

/// Configure the linear regression model to fit an intercept.
pub fn fit_intercept(mut self) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the name, because fit_intercept sounds like a special function of fit, but is actually part of the builder. Perhaps it would be easier to have a pub fn options(&mut self, options: Options) or alternatively with_intercept.

@bytesnake
Copy link
Member

bytesnake commented Jul 20, 2020

I will merge this PR now. If you have something to amend, please do so (otherwise any PR for Ridge/LASSO regression is welcome too 😄 )

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.

2 participants