Add support for tensor-valued spherical functions in interp_rbf#3237
Add support for tensor-valued spherical functions in interp_rbf#3237skoudoro merged 7 commits intodipy:masterfrom
interp_rbf#3237Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 |
|
I am not sure how to fix the two failing checks. |
|
Hi @kvttt, Thank you for working on this! I would recommend to create a new function named 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.
no
Yes
No, since we plan to remove this version and keep only your new version Can you update this? Thank you ! |
Concerning the checks, no worries concerning this 2 checks |
skoudoro
added
the
type:refactoring
…st for deprecation warnings
skoudoro
left a comment
skoudoro
left a comment
There was a problem hiding this comment.
Hi @kvttt,
Overall, it looks good. see below my last quick comments.
@arokem or @jhlegarreta, can you have a look?
dipy/core/interpolation.pyx
Outdated
| "dipy.core.interpolation.interp_rbf is deprecated, " | ||
| "Please use " | ||
| "dipy.core.interpolation.rbf_interpolation instead", | ||
| since="1.9", |
dipy/core/interpolation.pyx
Outdated
| "Please use " | ||
| "dipy.core.interpolation.rbf_interpolation instead", | ||
| since="1.9", | ||
| until="2.0", |
dipy/core/interpolation.pyx
Outdated
| def rbf_interpolation(data, sphere_origin, sphere_target, | ||
| function='multiquadric', epsilon=None, smoothing=0.1): |
There was a problem hiding this comment.
use keyword-only.
def rbf_interpolation(data, sphere_origin, sphere_target, *, function='multiquadric', epsilon=None, smoothing=0.1): instead
dipy/core/interpolation.pyx
Outdated
| sphere_target : Sphere | ||
| M positions on the unit sphere where the spherical function is interpolated. | ||
|
|
||
| function : {'linear', 'thin_plate_spline', 'cubic', 'quintic', 'multiquadric', 'inverse_multiquadric', |
There was a problem hiding this comment.
function: str, optional
Radial basis function. Possible values :{'linear', 'thin_plate_spline', 'cubic', 'quintic', 'multiquadric', 'inverse_multiquadric', 'inverse_quadratic', 'gaussian'}instead.
- no need of default
dipy/core/interpolation.pyx
Outdated
| function : {'linear', 'thin_plate_spline', 'cubic', 'quintic', 'multiquadric', 'inverse_multiquadric', | ||
| 'inverse_quadratic', 'gaussian'} | ||
| Radial basis function. Default: `thin_plate_spline`. | ||
| epsilon : float |
dipy/core/interpolation.pyx
Outdated
| Radial basis function spread parameter. | ||
| Defaults to 1 when `function` is 'linear', 'thin_plate_spline', 'cubic', or 'quintic'. | ||
| Otherwise, `epsilon` must be specified. | ||
| smoothing : float |
dipy/core/interpolation.pyx
Outdated
| scipy.interpolate.RBFInterpolator | ||
|
|
||
| """ | ||
| from scipy.interpolate import RBFInterpolator |
There was a problem hiding this comment.
Why do we import this here and not at the top of the file?
I need to check but can you look at it also ?
There was a problem hiding this comment.
I don't think there any good reason to put the import in here (there's a high likelihood I originally did this).
There was a problem hiding this comment.
Hi @kvttt .
Can you address thislast comment? Then it will be ready to go
Thanks!
dipy/core/interpolation.pyx
Outdated
| """ | ||
| from scipy.interpolate import RBFInterpolator | ||
|
|
||
| last_dim_idx = len(data.shape) - 1 |
There was a problem hiding this comment.
len(data.shape) == data.ndim. can you update?
|
Thank you for the quick update @kvttt! is there any additional comment ? @arokem ? @Garyfallidis ? someone else ? |
arokem
left a comment
arokem
left a comment
There was a problem hiding this comment.
Overall, looks good to me!
dipy/core/interpolation.pyx
Outdated
| scipy.interpolate.RBFInterpolator | ||
|
|
||
| """ | ||
| from scipy.interpolate import RBFInterpolator |
There was a problem hiding this comment.
I don't think there any good reason to put the import in here (there's a high likelihood I originally did this).
3 participants
Fixes #3236.
This PR adds support for tensor-valued spherical functions in
interp_rbf. When the user calls this function, specifyinglegacy=Trueinternally usesscipy.interpolate.Rbfwhich is the original behavior; specifyinglegacy=Falseinternally usesscipy.interpolate.RBFInterpolator, which allowsdatato have arbitrary dimensions.It would be great if I could get some feedbacks on the following questions:
normis already pending deprecation and is not used inscipy.interpolate.RBFInterpolator. As of my initial commit,normis ignored whenlegacy=False. Do we still want to keepnorm?functionis said to be one of{'multiquadric', 'inverse', 'gaussian'}, but bothscipy.interpolate.Rbfandscipy.interpolate.RBFInterpolatorsupport a little bit more than that (see here and here). Should I change the docstring?Rbf, whenepsilon=None, it internally estimate the average distance between nodes. However, inRBFInterpolator, this behavior is gone. The user has to specifyepsilonwhenkernelis not one of{'linear', 'thin_plate_spline', 'cubic', or 'quintic'}. Should I change the docstring to reflect this new behavior?