Skip to content

MAINT Replaces cnp.ndarray with memory views in _cd_fast #25775

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

MAINT Replaces cnp.ndarray with memory views in _cd_fast #25775

jjerphan merged 4 commits into from
Mar 9, 2023

Conversation

@OmarManzoor
Copy link
Contributor

@OmarManzoor OmarManzoor commented Mar 7, 2023 •
edited
Loading

Reference Issues/PRs

Towards #25484
Towards #24875

What does this implement/fix? Explain your changes.

  • Replace cnp.ndarray with const memory views in _cd_fast.
  • Remove the -Wcpp warning when compiling _cd_fast.

Any other comments?

CC: @thomasjpfan @jjerphan

@OmarManzoor OmarManzoor changed the title MAINT Replaces cnp.ndarray with memory views in _cd_fast and remove -… MAINT Replaces cnp.ndarray with memory views in _cd_fast Mar 7, 2023
jjerphan
jjerphan reviewed Mar 7, 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.

Thank you, @OmarManzoor.

Here are a few comments. I think this LGTM, I am just wondering if we can make the code more natural without having casts.

jeremiedbb
jeremiedbb approved these changes Mar 8, 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

@jeremiedbb
Copy link
Member

jeremiedbb commented Mar 8, 2023

you probably can remove the cnp.import_array() at the top of the file now

@jjerphan
Copy link
Member

jjerphan commented Mar 9, 2023

Following discussions in #25780 (review), can you reintroduce the call to cnp.import_array.

@jjerphan jjerphan enabled auto-merge (squash) March 9, 2023 08:01
@jjerphan jjerphan merged commit 63ff303 into scikit-learn:main Mar 9, 2023
@OmarManzoor OmarManzoor deleted the cython_cd_fast branch March 9, 2023 08:46
@OmarManzoor OmarManzoor mentioned this pull request Mar 9, 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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@OmarManzoor @jeremiedbb @jjerphan