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

[RF] remove cpdef in PmfGen #3063

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Conversation

skoudoro
Copy link
Member

@skoudoro skoudoro commented Feb 9, 2024

This PR is a small refactoring to remove the cpdef in pmf.pyx.

cpdef = def + cdef , so it does not make sense to have cpdef + cdef. it is either def +cdef or cpdef

I also use the opportunity to replace some pointer by memoryview. When using memoryview, we should make sure to use aligned memory by using [::1] for speed reason.

@gabknight, can you have a look and try?

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (916757c) 82.37% compared to head (aa1768d) 82.37%.
Report is 5 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3063   +/-   ##
=======================================
  Coverage   82.37%   82.37%           
=======================================
  Files         146      146           
  Lines       20631    20631           
  Branches     3319     3319           
=======================================
  Hits        16995    16995           
  Misses       2805     2805           
  Partials      831      831           

@gabknight
Copy link
Contributor

LGTM. This makes the code much nicer, thanks. Quick testing show no performance changes.

@skoudoro
Copy link
Member Author

Thank you for the feedback! So I am going ahead and merge it

@skoudoro skoudoro merged commit e44de73 into dipy:master Feb 15, 2024
27 checks passed
@skoudoro skoudoro deleted the remove-cpdef-pmf branch February 15, 2024 19:33
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.

None yet

2 participants