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] Common Private Loss Module with tempita #20567

Merged
merged 133 commits into from
Nov 26, 2021

Conversation

lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented Jul 19, 2021

Reference Issues/PRs

Solves #15123.
This is an altenative to #19088. This PR uses Cython Tempita to generate Cython code from a template as is already done in _sag_fast.pyx.tp and _seq_dataset.pyx.tp.

See #19088 (comment), issue #15123 and file _loss.pyx.tp (# Design:) for design choices.
Using Tempita helps reducing repeating code patterns for loops over samples in order to guarantee inlining.

@ogrisel
Copy link
Member

ogrisel commented Jul 26, 2021

I suppose that we should get the same performance as in #19088 because in the end the generated code should be equivalent, right?

@lorentzenchr
Copy link
Member Author

All 🟢 again!!!

@lorentzenchr
Copy link
Member Author

Technically, there are 2 reviewers' approval. @thomasjpfan If you are also good with it, we could be close to merge?

BWT, thanks for convincing me of composition over inheritance. I should have read Design Patterns (1994) by the GoF more carefully. In particular, we avoid to rely on Cython multiple inheritance.

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.

Second seal of approval since my last review: this LVGTM! Thank you, @lorentzenchr.

I think this PR has been standing still without attention for a long time.
I believe this can be merged but I would for other reviewers' approval.

@rth
Copy link
Member

rth commented Nov 26, 2021

I believe this can be merged but I would for other reviewers' approval.

+1 for merging with 2 approvals, particularly since it doesn't affect any of the estimators yet and is a private API.

@lorentzenchr
Copy link
Member Author

I believe this can be merged but I would for other reviewers' approval.

+1 for merging with 2 approvals, particularly since it doesn't affect any of the estimators yet and is a private API.

If only someone would press that button 🙏 Preferably, it's not the committer of the PR.

@rth rth merged commit d09e1d7 into scikit-learn:main Nov 26, 2021
@lorentzenchr
Copy link
Member Author

@rth ❤️

@lorentzenchr lorentzenchr deleted the loss_module_tempita branch November 26, 2021 16:11
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants