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

Implements linear_model.LinearRegression #2260

Merged
merged 34 commits into from Aug 31, 2021

Conversation

Fernadoo
Copy link
Contributor

What do these changes do?

Here is a draft PR.

  • Implement learn.linear_model.LinearRegression.
  • Align with sklearn::0.24.x.
  • Replace spicy.lstsq with closed form solutions via mars.tensor, but still cannot catch LinAlgError by singular matrices
  • Thus only pass 18/30 tests, 3 of which are highly related to sparse/csr issues.
  • Remain many np/scipy operations to be replaced.

Related issue number

@Fernadoo Fernadoo changed the title Fengming linear_model.LinearRegression Jul 29, 2021
@hekaisheng
Copy link
Contributor

Thanks for your contribution, could you add licenses at the head of new files?

@Fernadoo
Copy link
Contributor Author

Thanks for your contribution, could you add licenses at the head of new files?

Sure

@Fernadoo
Copy link
Contributor Author

Fernadoo commented Aug 2, 2021

Hi @hekaisheng, so far I have added license for new files and replaced np operators with mt operators. The pytest suggests '16 failed, 14 passed, 10 warnings in 57.04s'.
Still on the way of debugging 😵

@hekaisheng
Copy link
Contributor

Hi @hekaisheng, so far I have added license for new files and replaced np operators with mt operators. The pytest suggests '16 failed, 14 passed, 10 warnings in 57.04s'.
Still on the way of debugging 😵

Great! I will take a look at it in this week.

@qinxuye qinxuye added mod: learn to be backported Indicate that the PR need to be backported to stable branch type: feature New feature labels Aug 5, 2021
@qinxuye qinxuye added this to In progress in Learn via automation Aug 5, 2021
@qinxuye qinxuye added this to PR-In progress in v0.8 Release via automation Aug 5, 2021
@qinxuye qinxuye added this to the v0.8.0a2 milestone Aug 5, 2021
@Fernadoo
Copy link
Contributor Author

Fernadoo commented Aug 9, 2021

Have passed all tests except sparse/csr related functionalities.

@Fernadoo
Copy link
Contributor Author

Fernadoo commented Aug 9, 2021

Hi @hekaisheng, can I extend job timeout threshold to something longer, like 150min?, https://github.com/mars-project/mars/pull/2260/checks?check_run_id=3279790160

@hekaisheng
Copy link
Contributor

Hi @hekaisheng, can I extend job timeout threshold to something longer, like 150min?, https://github.com/mars-project/mars/pull/2260/checks?check_run_id=3279790160

The test job for learn costs less than 60 minutes on master, it seems unreasonable that your cases need more than one hour

@Fernadoo
Copy link
Contributor Author

Fernadoo commented Aug 9, 2021

Hi @hekaisheng, can I extend job timeout threshold to something longer, like 150min?, https://github.com/mars-project/mars/pull/2260/checks?check_run_id=3279790160

The test job for learn costs less than 60 minutes on master, it seems unreasonable that your cases need more than one hour

Oh sorry I didnt see your msg

@Fernadoo
Copy link
Contributor Author

Fernadoo commented Aug 9, 2021

Hi @hekaisheng, can I extend job timeout threshold to something longer, like 150min?, https://github.com/mars-project/mars/pull/2260/checks?check_run_id=3279790160

The test job for learn costs less than 60 minutes on master, it seems unreasonable that your cases need more than one hour

Oh sorry I didnt see your msg

It runs for less then 20mins in my local laptop, not sure what's going on here, lets just re-run the tests

@hekaisheng
Copy link
Contributor

It's better to use the fixture setup for tests, just pass setup to your test functions. For example:

def test_linear_regression(setup):

Copy link
Contributor

@hekaisheng hekaisheng left a comment

Choose a reason for hiding this comment

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

I left some comments and please add some cases to increase the coverage.

mars/learn/base.py Outdated Show resolved Hide resolved
mars/learn/base.py Outdated Show resolved Hide resolved
mars/learn/linear_model/_base.py Outdated Show resolved Hide resolved
mars/learn/linear_model/_base.py Outdated Show resolved Hide resolved
mars/learn/linear_model/_base.py Show resolved Hide resolved
mars/learn/metrics/_regression.py Outdated Show resolved Hide resolved
mars/learn/metrics/_regression.py Outdated Show resolved Hide resolved
mars/learn/preprocessing/normalize.py Outdated Show resolved Hide resolved
mars/learn/utils/sparsefuncs.py Outdated Show resolved Hide resolved
mars/learn/utils/sparsefuncs.py Outdated Show resolved Hide resolved
Learn automation moved this from In progress to Needs review Aug 11, 2021
v0.8 Release automation moved this from PR-In progress to PR-Needs review Aug 11, 2021
@Fernadoo Fernadoo force-pushed the fengming branch 2 times, most recently from ecac73a to dc1201b Compare August 13, 2021 08:38
@Fernadoo
Copy link
Contributor Author

Hi @hekaisheng, it seems all good 😇. Anything else to refine? Also, do I need to open another non-draft PR or you can directly merge this one?

Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hekaisheng hekaisheng left a comment

Choose a reason for hiding this comment

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

LGTM

Learn automation moved this from Needs review to Reviewer approved Aug 31, 2021
@hekaisheng hekaisheng merged commit 58d00be into mars-project:master Aug 31, 2021
Learn automation moved this from Reviewer approved to Done Aug 31, 2021
v0.8 Release automation moved this from PR-Needs review to PR-Done Aug 31, 2021
wjsi pushed a commit to wjsi/mars that referenced this pull request Sep 1, 2021
hekaisheng pushed a commit that referenced this pull request Sep 1, 2021
Co-authored-by: Fernando <42331572+Fernadoo@users.noreply.github.com>
@qinxuye qinxuye added backported already PR has been backported and removed to be backported Indicate that the PR need to be backported to stable branch labels Sep 3, 2021
@Fernadoo Fernadoo deleted the fengming branch September 6, 2021 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported already PR has been backported mod: learn type: feature New feature
Projects
Learn
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants