Skip to content

/ scikit-learn

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

Jump to bottom

[MRG+3] Fused types for MultiTaskElasticNet #8061

Merged
merged 8 commits into from Dec 28, 2016
Merged

[MRG+3] Fused types for MultiTaskElasticNet #8061

merged 8 commits into from Dec 28, 2016

Conversation

@tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Dec 15, 2016 •
edited

Reference Issue

This PR is a continuation of the work initiate by #6913.

What does this implement/fix? Explain your changes.

I've modified MultiTaskElasticNet to allow np.float32 for the computation.

Here are the memory profiling results when fitting np.float32 data:

  • master
    master

  • this branch
    branch

@jnothman @amueller Do you have some time to review that PR please :) ? It's big because of the cblas file I needed to add.
@tguillemot
tguillemot reviewed Dec 15, 2016
View changes
sklearn/src/cblas/cblas_sscal.c Outdated
* University of Tennessee - Innovative Computing Laboratory
* Knoxville TN, 37996-1301, USA.
*
* ---------------------------------------------------------------------

This comment has been minimized.

Sign in to view
@tguillemot

tguillemot Dec 15, 2016
Author Contributor

I have modified cblas_dscal.c to obtain this one. I prefer to find the original version of that file but actually in ATLAS they use an older version of that file.

@fabianp Where have you found the cblas_dscal.c ?

This comment has been minimized.

Sign in to view
@fabianp

fabianp Dec 19, 2016
Member

I got it from the ATLAS distribution. They are all taken from "{ATLAS}/src/blas/reference/level*". In the level1 directory I see a ATL_srefscal.c that looks to me very similar to the one on this PR. Is this the one you say is "older"?

This comment has been minimized.

Sign in to view
@tguillemot

tguillemot Dec 20, 2016
Author Contributor

I had a look to the cblas_dscal.c file and I found it in "{ATLAS}/interfaces/blas/C/src".
It's the only place where I find it.
Is the sklearn version of cblas_dscal.c was a modified version of ATL_drefscal.c ?
When I compare the files it seems to be that indeed.

Consequently I will do the same for cblas_sscal.c.
Thx @fabianp
tguillemot added 7 commits Dec 9, 2016
@tguillemot
Convert ElasticNet Multioutput to floating.
9be698c
@tguillemot
Remove all the float64 coordinate_descent.
cab308c
@tguillemot
Add the necessary cblas for use fused types.
79ab1b3
@tguillemot
Fix zeros dtype issue in cd_fast.
7935036
@tguillemot
Remove cblas files.
2135951
@tguillemot
Change random seed to let test_lle_simple_grid pass.
cdaf0c6
@tguillemot
Add tests to check floatting issue for MultiTaskElasticNet.
Loading status checks…
74d1e7e
@tguillemot tguillemot force-pushed the tguillemot:floating_elasticnet branch from c3121e5 to 74d1e7e Dec 15, 2016
@raghavrv
Copy link
Member

@raghavrv raghavrv commented Dec 15, 2016

Ping myself I'll review this in a while. (This weekend hopefully, Masters application is consuming a lot of my time :/)
@tguillemot
Copy link
Contributor Author

@tguillemot tguillemot commented Dec 15, 2016

Don't worry @raghavrv, this PR will not escape :).
Good luck for masters application.
@amueller
Copy link
Member

@amueller amueller commented Dec 16, 2016

This looks sweet, but probably not this year :-/
@agramfort
Copy link
Member

@agramfort agramfort commented Dec 17, 2016

looks clean to me. Tests pass and there is no algorithmic change.

+1 for merge

nice job @tguillemot
@agramfort agramfort changed the title [MRG] Fused types for MultiTaskElasticNet [MRG+1] Fused types for MultiTaskElasticNet Dec 17, 2016
@tguillemot
Copy link
Contributor Author

@tguillemot tguillemot commented Dec 19, 2016

Thx @agramfort.
Maybe @raghavrv @TomDLT if you have a few time ?
@TomDLT
Copy link
Member

@TomDLT TomDLT commented Dec 19, 2016 •
edited

LGTM, except for this comment #8061 (comment).
We need either the original file, or a comment in the file explaining your change.

ping @fabianp
@raghavrv
Copy link
Member

@raghavrv raghavrv commented Dec 19, 2016

Looks good to me as well. Amazing job! Thx
@raghavrv
raghavrv approved these changes Dec 19, 2016
View changes
@agramfort
Copy link
Member

@agramfort agramfort commented Dec 21, 2016

@tguillemot tell us when blas issue is sorted. If it is +1 for merge
@TomDLT TomDLT changed the title [MRG+1] Fused types for MultiTaskElasticNet [MRG+3] Fused types for MultiTaskElasticNet Dec 21, 2016
@tguillemot
Update cblas_sscal.c
Loading status checks…
cea699d
@raghavrv
Copy link
Member

@raghavrv raghavrv commented Dec 28, 2016

cblas_sscal.c is updated and all tests seem to pass cleanly. Thx @tguillemot... Merging this...
@raghavrv raghavrv merged commit fcb706a into scikit-learn:master Dec 28, 2016
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tguillemot
Copy link
Contributor Author

@tguillemot tguillemot commented Dec 28, 2016

@raghavrv You are faster than my messages. Thanks.

Sorry for the delay everyone.
sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@tguillemot @sergeyf
[MRG+3] Fused types for MultiTaskElasticNet (scikit-learn#8061)
4a90032 
* Convert ElasticNet Multioutput to floating.

* Remove all the float64 coordinate_descent.

* Add the necessary cblas for use fused types.

* Fix zeros dtype issue in cd_fast.

* Remove cblas files.

* Change random seed to let test_lle_simple_grid pass.

* Add tests to check floatting issue for MultiTaskElasticNet.

* Update cblas_sscal.c
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Closed
Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@tguillemot @Sundrique
[MRG+3] Fused types for MultiTaskElasticNet (scikit-learn#8061)
b3cf058 
* Convert ElasticNet Multioutput to floating.

* Remove all the float64 coordinate_descent.

* Add the necessary cblas for use fused types.

* Fix zeros dtype issue in cd_fast.

* Remove cblas files.

* Change random seed to let test_lle_simple_grid pass.

* Add tests to check floatting issue for MultiTaskElasticNet.

* Update cblas_sscal.c
NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@tguillemot @NelleV
[MRG+3] Fused types for MultiTaskElasticNet (scikit-learn#8061)
0410331 
* Convert ElasticNet Multioutput to floating.

* Remove all the float64 coordinate_descent.

* Add the necessary cblas for use fused types.

* Fix zeros dtype issue in cd_fast.

* Remove cblas files.

* Change random seed to let test_lle_simple_grid pass.

* Add tests to check floatting issue for MultiTaskElasticNet.

* Update cblas_sscal.c
paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@tguillemot @paulha
[MRG+3] Fused types for MultiTaskElasticNet (scikit-learn#8061)
fa738ab 
* Convert ElasticNet Multioutput to floating.

* Remove all the float64 coordinate_descent.

* Add the necessary cblas for use fused types.

* Fix zeros dtype issue in cd_fast.

* Remove cblas files.

* Change random seed to let test_lle_simple_grid pass.

* Add tests to check floatting issue for MultiTaskElasticNet.

* Update cblas_sscal.c
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@tguillemot @maskani-moh
[MRG+3] Fused types for MultiTaskElasticNet (scikit-learn#8061)
72ad874 
* Convert ElasticNet Multioutput to floating.

* Remove all the float64 coordinate_descent.

* Add the necessary cblas for use fused types.

* Fix zeros dtype issue in cd_fast.

* Remove cblas files.

* Change random seed to let test_lle_simple_grid pass.

* Add tests to check floatting issue for MultiTaskElasticNet.

* Update cblas_sscal.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianp fabianp

@raghavrv raghavrv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@tguillemot @raghavrv @amueller @agramfort @TomDLT @fabianp