Skip to content

MAINT Refactor the common logic for GEMM in wrapper #22719

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

Merged
merged 5 commits into from
Mar 11, 2022

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Mar 7, 2022

Reference Issues/PRs

Follow-up of #22320.
Precedes #22590.

What does this implement/fix? Explain your changes.

As we are going to introduce more and more concrete PairwiseDistancesReduction, the logic for the GEMM calls for the specialization is going to duplicated again and again.

A component is introduced here to help removing some of the duplicated code logic for GEMM.

Moreover, this introduction will also help with the upcasting when working on the support of new dtypes as the templating will only be done on this component.

This also convert the previous buffers to std::vectors for an eased memory management.

As we are going to introduce more and more concrete
`PairwiseDistancesReduction`, this will help removing
duplicated code logic.

Moreover, this will also help with the upcast when
templating implementations to support various dtypes.
@jjerphan jjerphan changed the title MAINT Wrap the common logic for GEMM under a wrapper MAINT PairwiseDistancesReduction: factor the common logic for GEMM under a wrapper Mar 7, 2022
@jjerphan jjerphan marked this pull request as ready for review March 7, 2022 14:38
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Overall, I like the refactor.

@jjerphan jjerphan changed the title MAINT PairwiseDistancesReduction: factor the common logic for GEMM under a wrapper MAINT PairwiseDistancesReduction: factor the common logic for GEMM in wrapper Mar 7, 2022
@jjerphan jjerphan requested a review from thomasjpfan March 11, 2022 10:27
@ogrisel ogrisel added the Quick Review For PRs that are quick to review label Mar 11, 2022
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!

Just a renaming suggestion for the compo attribute and some more doc to make the purpose of the new component more explicit.

jjerphan and others added 4 commits March 11, 2022 15:35
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan changed the title MAINT PairwiseDistancesReduction: factor the common logic for GEMM in wrapper MAINT Refactor the common logic for GEMM in wrapper Mar 11, 2022
@thomasjpfan thomasjpfan merged commit 6e313c0 into scikit-learn:main Mar 11, 2022
@jjerphan jjerphan deleted the pdr/gemm_component branch March 11, 2022 17:30
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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.

3 participants