convolve_fft semantic & functional mangling of fill_value

[jamienoss]

I think the code in convolution.convolve_fft() is being too clever for its own good.

At present, the following code snippets prevent fill_value=np.nan. This is semantically used for both boundary='fill' and nan_treatment='fill'.

[src]

..
   if not np.all(newshape == arrayshape):
        if np.isfinite(fill_value):
            bigarray = np.ones(newshape, dtype=complex_dtype) * fill_value
        else:
            bigarray = np.zeros(newshape, dtype=complex_dtype)
...

[src]

...
    if interpolate_nan:
        if not np.isfinite(fill_value):
            bigimwt = np.zeros(newshape, dtype=complex_dtype)
        else:
            bigimwt = np.ones(newshape, dtype=complex_dtype)
...

I think the user is being guarded to restrictively against user error and as a consequence, the functionality is no longer semantically intuitive and in at least one case incorrect.

E.g. convolve_fft(array, kernel, nan_treatment='fill', fill_value=np.nan), I would have assumed that any NaN values in the array would be replaced with NaN values, effectively doing nothing - however, that's the users choice, and is, therefore, user error. Therefore, an error should be raised instead of implicitly and opaquely dealing with it under the hood.

Furthermore, since fill_value is overloaded and used for both boundary='fill' and nan_treatment='fill', it should be at least possible to fill the boundary with NaN values and then for nan_treatment='interpolate', have the boundaries interpolated. This is not currently possible. As for the 1st point an error should be raised for the stateboundary='fill', nan_treatment='fill', fill_value=np.nan. The nan_treatment would need to be moved to after the boundaries are filled.

Ultimatley, two independent fill values are required.

[keflavich]

Agreed on the final point, and this was something we simply didn't think of when we were attempting to better match the convolve and convolve_fft arguments.

Generally, I agree with what you've laid out here; we should raise errors rather than silently correct bad options.

I don't quite understand: "it should be at least possible to fill the boundary with NaN values and then for nan_treatment='interpolate', have the boundaries interpolated." If the boundaries are filled with NaN, there is no distinction between 'fill' and 'interpolate' any longer (unless you allow a separate fill value internally, so maybe you're highlighting that case that is not yet supported at all?).

[jamienoss]

If the boundaries are filled with NaN, there is no distinction between 'fill' and 'interpolate' any longer

There is, well there should be but isn't currently. Also, I confess, I'll backpedal a little as I'm not sure that I was thinking correctly and whether I was making sense, but here is at least what I thought I meant.

convolve_fft(array, kernel, nan_treatment='interpolate', boundary='fill', fill_value=np.nan) here fill_value semantically belongs to boundary not nan_treatment. In this case, the boundaries should be filled with NaN values. Since nan_treatment='interpolate', these newly added NaN values will be picked up by the nan_treatment and dealt with accordingly. In the current implementation, that means zeroing them and weighting them after the fact. However, this doesn't happen because the fill values need to be finite and the NaN checking happens pre-filling.

Now I know that this might not make sense as a reasonable use case, especially since they're pixels in the boundary, which don't technically exist post convolution and therefore any interpolation isn't persisted. However, what it does indicate, is that the logic within the function is slightly mangled, there is scattered and implicit logic where there shouldn't be. E.g. the checking for NaN values for the NaN treatment, should technically happen post boundary filling.

Actually, I wonder if the above is how you would/could implement boundary=None.