A bug in the getBarycentric function in math_utils-inl.h file.

[ZeusYang]

Hello, doyubkim. It's me again. I got a bug in the getBarycentric function in math_utils-inl.h file. In the following code:

template<typename T>
inline void getBarycentric(
    T x,
    ssize_t iLow,
    ssize_t iHigh,
    ssize_t* i,
    T* f) {
    T s = std::floor(x);
    *i = static_cast<ssize_t>(s);

    ssize_t offset = -iLow;
    iLow += offset;
    iHigh += offset;

    if (iLow == iHigh) {
        *i = iLow;
        *f = 0;
    } else if (*i < iLow) {
        *i = iLow;
        *f = 0;
    } else if (*i > iHigh - 1) {
        *i = iHigh - 1;
        *f = 1;
    } else {
        *f = static_cast<T>(x - s);
    }

    *i -= offset;
}

At the end of the function, you let the i minus the offset which is no need for the last condition branch. In the last condition branch, i hasn't been modified which means i equals static_cast<ssize_t>(s), and s hasn't been subtracted by offset. It was missing from tests because in many cases iLow is 0 (so offset is 0 as well) . In fact, I don't think we need an offset and could just remove the offset to fix this bug.

template<typename T>
inline void getBarycentric(
    T x,
    ssize_t iLow,
    ssize_t iHigh,
    ssize_t* i,
    T* f) {
    T s = std::floor(x);
    *i = static_cast<ssize_t>(s);

    //ssize_t offset = -iLow;
    //iLow += offset;
    //iHigh += offset;

    if (iLow == iHigh) {
        *i = iLow;
        *f = 0;
    } else if (*i < iLow) {
        *i = iLow;
        *f = 0;
    } else if (*i > iHigh - 1) {
        *i = iHigh - 1;
        *f = 1;
    } else {
        *f = static_cast<T>(x - s);
    }

    //*i -= offset;
}

[doyubkim]

Thanks for looking into this, @ZeusYang ! In fact, this is more like a bad API design + lack of documentation.

Parameter x is a "relative distance" from iLow. Thus, you do need the offset compensation in the end. It is true, however, there's zero case where iLow is non-zero in the code other than the unit tests. So it might be possible to introduce additional function without iLow parameter, and deprecate the current one in the future. That will make the codebase a lot simpler and less confusing.

[ZeusYang]

@doyubkim Sorry, I haven't paid attention to unit tests. Parameter x is a "relative distance" from iLow sounds quite a bit weird. But anyway, it does make sense in this way. I did't mean to disturb you frequently. Thanks.

[doyubkim]

No problem at all, @ZeusYang. I actually think this is an API "bug." I will keep this issue open and fix the API.