Filtering NaN inputs in filter 1d

[awxkee]

As discussed #2838

[RunDevelopment]

I don't think panicking on zero or close-to-zero kernel sizes/sigmas is the best solution here. As sigma approaches zero, the image gets less and less blurred, so at sigma=0 the image shouldn't be blurred at all. I believe we should simply return an unmodified copy of the input image for sigmas below some threshold. (This is what OpenCV does too IIRC.)

Not sure what the behavior should be for sigma=NaN and sigma<0. Both panicking and returning the image unmodified seem fine to me.

[awxkee]

Sigma 0 is singularity https://en.wikipedia.org/wiki/Gaussian_blur, to avoid forcing user to set any sigma we have convenience methods.
OpenCV allows zero because it will compute sigma from kernel size, what we also do in parameters implementation.

Subnormals also have almost no point, since we're using f32 for the kernel; kernel computation will underflow, so the result is signed zero, so the convolving result is signed zero as well, with underflow which should be signalled. I don't think handling this in image library make sense.

[197g]

Panicking might be fine if we did it early. Panics further down the call stack should not be error handling. At least they should to be documented on the public entry points which I guess might be painful enough here to be convincing. Maybe we use a validating constructor instead? I do think that we don't want to silently swallow NaN sigma entirely. Negative could be treated with saturating to zero in my opinion, it isn't inconvenient if the passed sigma is calculated and the algorithm for calculating it is not entirely safe against floating-point rounding giving impossible, tiny negative values.

[awxkee]

Well, it appeared we're solving problem that doesn't exists since we didn't add public arbitrary constructor. I added one and comments.

PTAL

Negative could be treated with saturating to zero in my opinion, it isn't inconvenient if the passed sigma is calculated and the algorithm for calculating it is not entirely safe against floating-point rounding giving impossible, tiny negative values.

I didn't exactly get actually where to do it, because in convenience methods except new_from_radius we don't allow zero either. Did you mean to saturate radius to 0 in new_from_radius ?

[RunDevelopment]

Sigma 0 is singularity https://en.wikipedia.org/wiki/Gaussian_blur

That doesn't mean the operation doesn't have a limit.

If I'm reading the code correctly, the generated kernels are normalized to have a sum of 1. This means that all kernels of size 1 are simply [1.0]. Sigma doesn't matter. This kernel also performs no blur, since it's just the identity.

This should also mean that a bunch of small kernel sizes all behave identically. For new_from_kernel_size, any kernel size $\in(0,1.5)$ will map to {x,y}_axis_kernel_size=1. For new_from_radius, any radius $\in(0,0.25)$ will map to {x,y}_axis_kernel_size=1. I think this should be considered a bug in the current implementation of these functions. OpenCV behaves similarly, and I had to work around it there by making a 3x3 kernel for small blurs myself.

[awxkee]

This is not a bug it's plan. There was no plan to expose complete filtering API, only some reasonable defaults. As well OpenCV did because in the most cases no one needs that. And if anyone needs to handle different filtering kernels, they do need different implementation and API.

[RunDevelopment]

some reasonable defaults

It's not reasonable though. No one would expect that new_from_radius(0.51) performs a small blur and suddenly new_from_radius(0.5) performs no blur. The behavior should be continuous.

---

Also, I just tested it and small blur radii are completely broken. Any radius <= 0.5 performs no blur and moves all pixel 1 to the right. This should be addressed in a separate PR, though.

[awxkee]

Actually, we're fixing a problem that never existed, and we might just silently introduce one, so I'll go ahead and close this.

It's not reasonable though. No one would expect that new_from_radius(0.51) performs a small blur and suddenly new_from_radius(0.5) performs no blur. The behavior should be continuous.

I personally don't think so, this method does exactly what is does. If 0.49 rounds to kernel size 1, so I tend to think that user should handle this. I don't see why it should magically resolve 0.49 to a kernel of size 3 without any reason.

[RunDevelopment]

I don't see why it should magically resolve 0.49 to a kernel of size 3 without any reason.

Because the blur doesn't work.

The use case where I had to manually work around OpenCV was basically an image editing program. That program had a blur effect, like so many others do. Basically this from Paint.net:

A user reported to us that blurring with radius of 0.2 (or similar, can't remember) doesn't work.

Currently, any program using image and giving users control over the blur amount will have the same bug but worse. I don't think that's acceptable.

[fintelia]

I haven't been following the full context in this thread, but it seems [edit: potentially] reasonable to me to always use a kernel size >=3 if kernel size 1 is just a no-op

[awxkee]

I see this completely differently:

1. new_from_radius is straighforward method which maps radius to kernel by rule 2*radius+1 where minimum possible value 1, as we can do blur for size 1 which is no-op. It wasn't supposed to make any guesses or work like "we support everywhere kernel size 1, but minimum here is 3", or trying to guess any "user pleasant behavior".
2. If the minimum kernel size here is 3, then the other methods should panic on kernel size < 3 to be consistent. Which is a bit strange and breaking, because we allow that and can do that.
3. Constraining library implementations and APIs to something "user expected behavior" is an application domain problem. Libraries typically assumed to do what's requested, not something "user assumed", and if we can do "blur" for size 1 and logic maps to it, then we should do it. This is an application problem to map direct library behavior to "user expectations". If library doesn't expose implementation or API to solve it, application needs to find an additional library or make required implementation.

---

Please, if someone wants to discuss problems or questions that is not related to a PR; I don't have an issue with that, but consider opening an issue instead of creating a long discussion about things that are not supposed to be addressed in the PR. Anyone interested will be able to find the problem in the issues.