dipy icon indicating copy to clipboard operation
dipy copied to clipboard

Add support for tensor-valued spherical functions in `interp_rbf`

Open kvttt opened this issue 1 year ago • 2 comments

Fixes #3236.

This PR adds support for tensor-valued spherical functions in interp_rbf. When the user calls this function, specifying legacy=True internally uses scipy.interpolate.Rbf which is the original behavior; specifying legacy=False internally uses scipy.interpolate.RBFInterpolator, which allows data to have arbitrary dimensions.

It would be great if I could get some feedbacks on the following questions:

  • I saw the kwarg norm is already pending deprecation and is not used in scipy.interpolate.RBFInterpolator. As of my initial commit, norm is ignored when legacy=False. Do we still want to keep norm?
  • In the docstring, function is said to be one of {'multiquadric', 'inverse', 'gaussian'}, but both scipy.interpolate.Rbf and scipy.interpolate.RBFInterpolator support a little bit more than that (see here and here). Should I change the docstring?
  • In Rbf, when epsilon=None, it internally estimate the average distance between nodes. However, in RBFInterpolator, this behavior is gone. The user has to specify epsilon when kernel is not one of {'linear', 'thin_plate_spline', 'cubic', or 'quintic'}. Should I change the docstring to reflect this new behavior?

kvttt avatar May 21 '24 18:05 kvttt

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.67%. Comparing base (19ae6a6) to head (92bed94). Report is 7 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3237   +/-   ##
=======================================
  Coverage   83.67%   83.67%           
=======================================
  Files         153      152    -1     
  Lines       21381    21373    -8     
  Branches     3458     3458           
=======================================
- Hits        17890    17884    -6     
+ Misses       2629     2627    -2     
  Partials      862      862           

see 15 files with indirect coverage changes

codecov[bot] avatar May 21 '24 19:05 codecov[bot]

I am not sure how to fix the two failing checks.

kvttt avatar May 23 '24 19:05 kvttt

Hi @kvttt,

Thank you for working on this!

I would recommend to create a new function named rbf_interpolation and deprecate the interp_rbf by adding our decorator (see an example here)

I do not see any benefit to keep all this different behaviors (if legacy or not )and add complexity to the maintenance. 1 function = 1 feature/task if possible.

Do we still want to keep norm?

no

Should I change the docstring?

Yes

Should I change the docstring to reflect this new behavior?

No, since we plan to remove this version and keep only your new version

Can you update this? Thank you !

skoudoro avatar Jun 04 '24 11:06 skoudoro

I am not sure how to fix the two failing checks.

Concerning the checks, no worries concerning this 2 checks

skoudoro avatar Jun 04 '24 11:06 skoudoro

Thank you for the quick update @kvttt!

is there any additional comment ? @arokem ? @Garyfallidis ? someone else ?

skoudoro avatar Jun 05 '24 12:06 skoudoro