Skip to content

Resample #309

Closed
wants to merge 2 commits into from

4 participants

@jbpoline
NIPY developers member

Pass some ndimage keyword arguments to the resampling routines. Useful if you need default value to something else than 0 for instance. Not all cases are covered.

@alexis-roche

Why not using the double star idiom as in:

resample(..., **interp_param)

@jbpoline
NIPY developers member

The reason for not doing **params was that "order" parameter is already a named arguments - and you could also have order in the dict. I think it's clearer with named argument (no previously used or unused parameters). But that's minor to me if you think strongly about it.

@alexis-roche
NIPY developers member

Well, order could be implicitly included in interp_param without changing the function signature. I think that passing an explicit dictionary is not very pythonic, thus possibly confusing.

@bthirion bthirion commented on the diff Mar 22, 2014
nipy/algorithms/registration/resample.py
@@ -54,6 +54,11 @@ def resample(moving, transform=None, reference=None,
maps from world coordinates.
interp_order: int, optional
Spline interpolation order, defaults to 3.
+ interp_param: None or dict, optional
@bthirion
bthirion added a note Mar 22, 2014

Sorry, but I dislike this kind of argument for a function that does a relatively precise algorithmic operation. You should provide a list of explicit arguments instead of a dictionary. Otherwise, how can the use figure out what arguments to give ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@matthew-brett
NIPY developers member

Ok - it looks like the assembled company prefer adding the explicit

mode='constant', cval=1.

to the function signature instead of interp_params. Is that right?

@bthirion

Yes

@matthew-brett
NIPY developers member

JB - would you mind making those changes?

@matthew-brett
NIPY developers member

JB - is there any chance you will get to this soon?

@matthew-brett
NIPY developers member

Closing this one in favor of #359 (same PR with refactoring on top).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.