Skip to content

MAINT add const qualifications to required places in cython_blas and cd_fast #25791

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
jjerphan merged 2 commits into from
Mar 9, 2023
Merged

MAINT add const qualifications to required places in cython_blas and cd_fast #25791

jjerphan merged 2 commits into from
Mar 9, 2023

Conversation

@OmarManzoor
Copy link
Contributor

@OmarManzoor OmarManzoor commented Mar 9, 2023

Reference Issues/PRs

Follow up of #25775

What does this implement/fix? Explain your changes.

  • Specifies const definition for the required pointer parameters in relevant _cython_blas functions.
  • Specifies const definition for required pointer parameters in _cd_fast functions abs_max and diff_abs_max
  • Removes the [-Wincompatible-pointer-types-discards-qualifiers] warnings that were raised as a result of MAINT Replaces cnp.ndarray with memory views in _cd_fast #25775

Any other comments?

CC @jjerphan @jeremiedbb

@jeremiedbb
Copy link
Member

jeremiedbb commented Mar 9, 2023

There are a few BLAS routines where you did not add the const keyword, even if it could be added like asum and ger for instance. Is there a reason for that ?

@OmarManzoor
Copy link
Contributor Author

OmarManzoor commented Mar 9, 2023

There are a few BLAS routines where you did not add the const keyword, even if it could be added like asum and ger for instance. Is there a reason for that ?

I only changed the functions and the parameters which were needed to remove the warnings from cd_fast.

@jeremiedbb
Copy link
Member

jeremiedbb commented Mar 9, 2023

I only changed the functions and the parameters which were needed to remove the warnings from cd_fast.

I think it's worth unifying the whole cython_blas module at once

@OmarManzoor
Copy link
Contributor Author

OmarManzoor commented Mar 9, 2023

I only changed the functions and the parameters which were needed to remove the warnings from cd_fast.

I think it's worth unifying the whole cython_blas module at once

Sure!

jeremiedbb
jeremiedbb approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb 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 @OmarManzoor

jjerphan
jjerphan approved these changes Mar 9, 2023
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

@jjerphan jjerphan merged commit 131642b into scikit-learn:main Mar 9, 2023
@OmarManzoor OmarManzoor deleted the cython_update_blas_signatures branch March 9, 2023 13:58
@lesteve lesteve mentioned this pull request Mar 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjerphan jjerphan jjerphan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees

No one assigned

Labels

cython module:linear_model module:utils

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@OmarManzoor @jeremiedbb @jjerphan