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 buffer argument in pmf_gen.get_pmf_value(.) #3214

Merged
merged 1 commit into from May 15, 2024

Conversation

gabknight
Copy link
Contributor

This PR removed an unnecessary argument pmf_buffer added in PR #3211

This is a speed and memory improvement. Because get_pmf_value() returns a double, the computation can be done for a single point on the sphere, rather than the whole sphere, thus removing the need for the buffer.

@skoudoro
Copy link
Member

Hi @gabknight,

Can you rebase this PR? Thank you

@gabknight
Copy link
Contributor Author

Hi @skoudoro, rebase done. Let's see if all tests pass.

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.75%. Comparing base (b585dd6) to head (0a0111b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3214   +/-   ##
=======================================
  Coverage   83.75%   83.75%           
=======================================
  Files         153      153           
  Lines       21340    21340           
  Branches     3445     3445           
=======================================
  Hits        17873    17873           
  Misses       2611     2611           
  Partials      856      856           

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.

All good, and CI's happy. Errors are not related. Merging

@skoudoro skoudoro merged commit eb07428 into dipy:master May 15, 2024
28 of 31 checks passed
@skoudoro
Copy link
Member

thanks @gabknight !

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