Skip to content

MAINT Improve source generation using Tempita #20481

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 42 commits into from
Aug 9, 2021
Merged

MAINT Improve source generation using Tempita #20481

merged 42 commits into from
Aug 9, 2021

Conversation

mbatoul
Copy link
Contributor

@mbatoul mbatoul commented Jul 7, 2021 •
edited by rth
Loading

Reference Issues/PRs

Relates to #11000.
Supersedes #13358.
Closes #13358

What does this implement/fix? Explain your changes.

This work builds on top of @massich's work (see #13358):

WeightVector is used in #13346 and has attributes that cannot be fused.

scikit-learn/sklearn/utils/weight_vector.pxd

Lines 12 to 20 in 984871b

cdef np.ndarray w
cdef np.ndarray aw
cdef double *w_data_ptr
cdef double *aw_data_ptr
cdef double wscale
cdef double average_a
cdef double average_b
cdef int n_features
cdef double sq_norm

This PR uses Tempita to allow float32 float64.

@mbatoul mbatoul changed the title [WIP] MAINT Use Tempita for WeightVector [WIP] MAINT Use Tempita for WeightVector Jul 7, 2021
@mbatoul mbatoul mentioned this pull request Jul 7, 2021
Closed
@mbatoul
Copy link
Contributor Author

mbatoul commented Jul 7, 2021

Answering @NicolasHug's comments on #13358:

Sorry not familiar with tempita (and the tempita doc links are broken): why do you need get_dispatch? You can't simply iterate over the dtypes list?

I removed the get_dispatch function in the template files in this commit. It seems to work fine by simply iterating over the dtypes list. I have found nothing regarding why we would need the get_dispatch function. The Tempita documentation is still unaccessible though.

@jjerphan jjerphan self-requested a review July 13, 2021 07:20
jjerphan
jjerphan reviewed Jul 15, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @mbatoul, for pursuing this work.

The templating of WeightVector LGTM.

Here are some comments regarding changes and the scope of this PR.

@mbatoul mbatoul changed the title [WIP] MAINT Improve source generation using Tempita MAINT Improve source generation using Tempita Aug 3, 2021
@mbatoul mbatoul requested review from jjerphan and jeremiedbb August 3, 2021 09:28
jjerphan
jjerphan reviewed Aug 3, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Here are some final suggestions and a comment for adapting a numerical hard-coded value.

@glemaitre glemaitre self-requested a review August 3, 2021 13:41
@mbatoul mbatoul marked this pull request as ready for review August 3, 2021 13:42
ogrisel
ogrisel approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM once the following comments are addressed:

jjerphan
jjerphan approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @mbatoul for this contribution!

@jjerphan
Copy link
Member

jjerphan commented Aug 6, 2021 •
edited
Loading

setup.cfg needs to be edited similarly to .gitignore to list files to ignore:

scikit-learn/setup.cfg

Lines 65 to 70 in 81dde3a

[check-manifest]
# ignore files missing in VCS
ignore =
sklearn/linear_model/_sag_fast.pyx
sklearn/utils/_seq_dataset.pxd
sklearn/utils/_seq_dataset.pyx

@mbatoul mbatoul requested a review from jjerphan August 6, 2021 12:18
jjerphan
jjerphan approved these changes Aug 6, 2021
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Still LGTM, @mbatoul: no need to ask for a new review. 🙂

@mbatoul @jjerphan
Keep alphanumerical order of files in setup.cfg.
d52c676 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
ogrisel
ogrisel approved these changes Aug 6, 2021
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Still LGTM, just another suggestion:

glemaitre
glemaitre approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. Some small doc things otherwise everything looks good.

mbatoul and others added 2 commits August 9, 2021 21:08
@mbatoul @ogrisel
Stop storing reset_wscale_threshold in a variable.
033f3f4 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@mbatoul @glemaitre
Improve docstrings.
6abf315 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre glemaitre merged commit 4b8cd88 into scikit-learn:main Aug 9, 2021
@glemaitre
Copy link
Member

Thanks @mbatoul

@jjerphan jjerphan mentioned this pull request Aug 11, 2021
Merged
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@mbatoul @jjerphan
@ogrisel @glemaitre @samronsin
MAINT Improve source generation using Tempita (scikit-learn#20481)
545b896 
Co-authored-by: Joan Massich <sik@visor.udg.edu>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@jjerphan jjerphan mentioned this pull request Mar 10, 2022
Open
@adam2392 adam2392 mentioned this pull request Mar 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@jjerphan jjerphan jjerphan approved these changes

@ogrisel ogrisel ogrisel approved these changes

@jeremiedbb jeremiedbb Awaiting requested review from jeremiedbb

Assignees
No one assigned
Labels
cython module:linear_model module:utils No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mbatoul @jjerphan @glemaitre @ogrisel @jeremiedbb