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

Adding support for btens to multi_shell_fiber_response function #2956

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

karanphil
Copy link
Contributor

Hi!

This is a really small modification of reconst.mcsd.multi_shell_fiber_response , adding support for the b-tensor (btens) option. I used the new implementation of btens in gradient_table and single_tensor, so that the reconst.mcsd.multi_shell_fiber_response function now returns the response function according to the b-tensor shape of each inputed b-value. For example, I use this for multi-encoding msmt-CSD (see my paper for more details https://doi.org/10.1016/j.media.2022.102476) on Scilpy (I am a PhD student supervised by Maxime Descoteaux), and I had this version of the function in my code. It runs as usual when no btens is given, by setting every b-value as LTE (linear tensor encoding).

It would be very appreciated if this could go in the next release. @gabknight

@skoudoro
Copy link
Member

Hi @karanphil,

Thank you for this. As usual, can you add some basic test?

It can go to the next release if @RafaelNH or @arokem find time to review it.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #2956 (277fb83) into master (09e7231) will decrease coverage by 0.01%.
The diff coverage is 62.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2956      +/-   ##
==========================================
- Coverage   81.79%   81.78%   -0.01%     
==========================================
  Files         146      146              
  Lines       20401    20406       +5     
  Branches     3238     3240       +2     
==========================================
+ Hits        16686    16689       +3     
- Misses       2898     2900       +2     
  Partials      817      817              
Files Coverage Δ
dipy/reconst/mcsd.py 86.11% <62.50%> (-0.82%) ⬇️

... and 1 file with indirect coverage changes

@arnaudbore arnaudbore mentioned this pull request Oct 31, 2023
41 tasks
@skoudoro
Copy link
Member

skoudoro commented Nov 2, 2023

@RafaelNH or @arokem, Do you have any opinions concerning this PR?

Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

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

LGTM! I think we can merge this

@arnaudbore
Copy link

@skoudoro I think you can merge this one easily since @arokem approved it 👍

@skoudoro skoudoro merged commit 4b46f6a into dipy:master Nov 15, 2023
23 of 24 checks passed
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

4 participants