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

[MRG+3] Fused types for MultiTaskElasticNet #8061

Merged
merged 8 commits into from Dec 28, 2016

Conversation

tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Dec 15, 2016

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.

* University of Tennessee - Innovative Computing Laboratory
* Knoxville TN, 37996-1301, USA.
*
* ---------------------------------------------------------------------
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@raghavrv
Copy link
Member

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

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

@amueller
Copy link
Member

This looks sweet, but probably not this year :-/

@agramfort
Copy link
Member

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

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

@TomDLT
Copy link
Member

TomDLT commented Dec 19, 2016

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

Looks good to me as well. Amazing job! Thx

@agramfort
Copy link
Member

@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
@raghavrv
Copy link
Member

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
@tguillemot
Copy link
Contributor Author

@raghavrv You are faster than my messages. Thanks.

Sorry for the delay everyone.

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
* 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
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
* 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 pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
* 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 pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
* 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 pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants