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

NF - Added cython utility functions #2716

Merged
merged 8 commits into from Jan 31, 2023

Conversation

gabknight
Copy link
Contributor

Utility functions for PTT tracking PR #2596.

Is there a good way to add unit tests on these?

@skoudoro
Copy link
Member

Thanks @gabknight,

For the test, you can create a test_fast_numpy.py in utils/tests folder.

Inside the tests, check if np.dot is slower than your function and obtain the same result

@gabknight
Copy link
Contributor Author

ok, thanks. I needed to change the function declaration from cdef to cpdef for the tests. Cross is 25x faster, dot is 1.5x on the length-3 vectors. There will more gain when doing the tracking in c.

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #2716 (9393354) into master (551fa1c) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2716   +/-   ##
=======================================
  Coverage   83.83%   83.83%           
=======================================
  Files         131      131           
  Lines       18292    18293    +1     
  Branches     2979     2980    +1     
=======================================
+ Hits        15335    15336    +1     
  Misses       2220     2220           
  Partials      737      737           
Impacted Files Coverage Δ
dipy/tracking/streamline.py 93.83% <0.00%> (+0.02%) ⬆️

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Just waiting for the CI to finish and this PR is ready to be merged.

Thank you @gabknight

@skoudoro
Copy link
Member

SMALL NOTE

We need a system to allow pytest to run test on cdef files. In this file, cpdef are used just for the tests. This is not necessary. I suppose we need test as pyx files. Something to explore

@skoudoro skoudoro merged commit 13af40f into dipy:master Jan 31, 2023
@gabknight gabknight deleted the NF_add_fastnumpy_fcts branch December 8, 2023 21:44
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