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 - BootDirectionGetter #2947

Merged
merged 9 commits into from
Oct 23, 2023
Merged

RF - BootDirectionGetter #2947

merged 9 commits into from
Oct 23, 2023

Conversation

gabknight
Copy link
Contributor

@gabknight gabknight commented Oct 20, 2023

This PR prepares PmfGen for cython optimization.

  • Move bootPmfGen functionalities from dipy.direction.pmf to dipy.direction.bootstrap_direction_getter.pyx
  • Change BootDirectionGetter parent class from PmfGenDirectionGetter to DirectionGetter
  • Remove bootPmfGen class from dipy.direction.pmf
  • Update tests

There are no noticeable performance impacts.

@pep8speaks
Copy link

pep8speaks commented Oct 20, 2023

Hello @gabknight, Thank you for updating !

Line 235:5: E303 too many blank lines (2)
Line 247:1: W391 blank line at end of file

Comment last updated at 2023-10-20 20:05:41 UTC

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #2947 (b278fb4) into master (3ad9ec1) will increase coverage by 0.01%.
Report is 97 commits behind head on master.
The diff coverage is 80.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2947      +/-   ##
==========================================
+ Coverage   81.77%   81.79%   +0.01%     
==========================================
  Files         146      146              
  Lines       20401    20401              
  Branches     3238     3238              
==========================================
+ Hits        16682    16686       +4     
+ Misses       2901     2898       -3     
+ Partials      818      817       -1     
Files Coverage Δ
dipy/data/fetcher.py 40.25% <ø> (ø)
dipy/info.py 100.00% <100.00%> (ø)
dipy/io/stateful_tractogram.py 76.96% <100.00%> (ø)
dipy/reconst/dti.py 92.57% <0.00%> (ø)
dipy/tracking/local_tracking.py 94.44% <96.29%> (ø)
dipy/io/utils.py 88.83% <90.00%> (ø)
dipy/io/streamline.py 84.92% <75.00%> (ø)
dipy/align/imwarp.py 95.60% <37.50%> (ø)

... and 1 file with indirect coverage changes

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.

Great! thank you for refactoring this! I like the simplification and the gain of speed.

it looks good to me for now. Can you check that we do not break any examples ?

Thank you

@gabknight
Copy link
Contributor Author

gabknight commented Oct 20, 2023

It did break the example! The data array was missing a dtype check/transformation to float. I also added an optional b_tol parameter for more control.

If the tests are successful, all good on my side.

@skoudoro skoudoro merged commit 09e7231 into dipy:master Oct 23, 2023
23 of 24 checks passed
@skoudoro
Copy link
Member

Thank you for this work @gabknight , merging

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

3 participants