@pavanky I wish I saw the PR before it got merged...
If I'm not mistaken with the merged implementation, at least for the CPU version, the missing value case (i.e. when none of the masks are set) the result will be set to numeric_limits<T>::max() instead of numeric_limits<T>::infinity(). For example for float the former is 3.40282e+38, while the latter is inf. This might be problematic for a few reasons
- since the value will be a real number, subsequent operations on it will not trigger any special inf-arithmetic, so it will be very hard to detect that something went wrong after
morph()was called. - I understand that ArrayFire doesn't need to replicate Matlab behaviour, but I think the fact that the result will be different than inf, which is what Matlab does, will cause more confusion, especially because it's not really obvious why
3.40282e+38was returned. - in many situations, after dilate() is called, people do something like this
out = dilate(in, filter);
out(isInf(out)) = 0;If we use max instead of inf, this type of code will be messier to write.
So, I was thinking, ideally we should let the user pass an optional missingValue argument to these functions. By default, if the missingValue is not given, it should use the special initial value (but still with inf for floats). I'm not sure if c++ allows passing template dependent default arguments to a function. If that's not possible, we might need to split these functions into two:
template<typename T, bool IsDilation> void morph(Array<T> out, Array<T> const in, Array<T> const mask, T missingValue) { } template<typename T, bool IsDilation> void morph(Array<T> out, Array<T> const in, Array<T> const mask) { T missingValue = ... // -inf/inf for floats, min/max for integrals morph(out, int, mask, missingValue); }
Note that, for the first function, since the passed missingValue still cannot be used as the initial value, we would have to keep a boolean when we do the inner loop:
T filterResult = init; bool missing = true; for (wj) { for (wi) { if (...) { missing = false; // ... } } } outData[ getIdx(ostrides, i, j) ] = missing ? missingValue : filterResult;
The nice thing about having this default missingValue, it allows the user not to write the messy and expensive code as in 3 above, instead it just becomes
out = dilate(in, filter, 0); // no need to worry about replacing inf or max anymore