Fix point<T> divide and multiply to not to hardcode result as point<double> #157

mloskot · 2018-10-21T18:36:11Z

The free variants of operator/ and operator* now yield new type of point<T> with T promoted to type common for both operands.

Add missing member operator*=, for symmetry.
Add template function iround(point<T>) as no-op sink for point specialisations for T of integer types.
Add explicit casts of arithmetic intermediate results to avoid compilation warnings.
Replace redundant point<T> construction in return statement with uniform initialization.
Add static_assert validating range of axis_value integer template parameter.

Update test/point/point.cpp with basic checks of all point<T> members.

References

Fixes #153

Tasklist

Review
Adjust for comments
All CI builds and checks have passed

mloskot · 2018-10-21T19:24:19Z

This PR is causing failures of the old friend test/image.cpp checksum checks, on both

AppVeyor https://ci.appveyor.com/project/stefanseefeld/gil/builds/19676513/job/xadppbxh7v8a54q2

testing.capture-output bin.v2\libs\gil\test\image.test\msvc-14.1\debug\address-model-64\asynch-exceptions-on\threading-multi\image.run
====== BEGIN OUTPUT ======
Reading checksums from libs\gil\test\gil_reference_checksums.txt
Checking checksum for bgr8_basic_red_x (crc=7f6e24e7)
Checking checksum for bgr8_basic_white_x (crc=e4aaa1d3)
Checking checksum for bgr8_views_original (crc=423f1dde)
Checksum error in bgr8_views_cropped (crc=760a7319 != ff56b05e)
Unknown exception
 
EXIT STATUS: 1 
====== END OUTPUT ======

Travis CI - https://travis-ci.org/mloskot/gil/builds/444434819

testing.capture-output bin.v2/libs/gil/test/image.test/gcc-7.3.0/debug/visibility-hidden/image.run
====== BEGIN OUTPUT ======
Reading checksums from libs/gil/test/gil_reference_checksums.txt
Checking checksum for bgr8_basic_red_x (crc=7f6e24e7)
Checking checksum for bgr8_basic_white_x (crc=e4aaa1d3)
Checking checksum for bgr8_views_original (crc=423f1dde)
Checksum error in bgr8_views_cropped (crc=760a7319 != ff56b05e)
std::exception
EXIT STATUS: 1
====== END OUTPUT ======

On AppVeyor, we are expecting some image checksum failures, eg. https://ci.appveyor.com/project/stefanseefeld/gil/builds/19660664, but these above are different.

I'll have to investigate what's going on

include/boost/gil/point.hpp

stefanseefeld · 2018-10-21T19:36:07Z

Interesting ! As far as I can see, the only semantic change of this PR changes the * and / operators to accept additional operand types (and then adjust the respective return types appropriately).
So it sounds like this change causes a different point type to be instantiated in some test (point<float>, presumably ?), which then triggers some other type conversion(s) leading to the computational (numeric ?) error we observe.
So with a bit of luck this coincidence will help us understand the root cause of the checksum error, right ?

mloskot · 2018-10-21T21:20:51Z

The problem was in the unexpected rounding or truncating towards zero like in here:

int x = 69;
int ups = x / 2; // 34

while the test/image.cpp uses of point<T> actually rely on the fact that point division yields point<double>, ie.:

check_view(subimage_view(img_view, iround(img_view.dimensions()/4), iround(img_view.dimensions()/2)),prefix+"cropped");

what effectively means:

point<int> p(69, 36);
point<double> pa = p / 2; // x = 34.5
iround(pa);               // x = 35

Originally I submitted this version of operator/ which, due to truncation towards zero, was not equivalent to point<double>, thus changed the behaviour (so, good catch by the test/image.cpp :-)):

template <typename T, typename D>
auto operator/(point<T> const& p, D d) -> point<typename std::common_type<T, D>::type>
{
    static_assert(std::is_arithmetic<D>::value, "denominator is not arithmetic type");
    using result_type = typename std::common_type<T, D>::type;
    if (d < 0 || 0 < d)
    {
        return point<result_type>{
            static_cast<result_type>(p.x) / d,
            static_cast<result_type>(p.y) / d};
    }
    else
    {
        return point<result_type>{0, 0};
    }
}

I've just fixed it this way to preserve the expected behaviour:

    if (d < 0 || 0 < d)
    {
        double const x = p.x / static_cast<double>(d);
        double const y = p.y / static_cast<double>(d);
        return point<result_type>{
            static_cast<result_type>(iround(x)),
            static_cast<result_type>(iround(y))};
    }

I think, this experience helps to get closer to the question asked in #153 about why authors of the original point<double> operator/ implemented it that way.

I think uses like iround(img_view.dimensions()/4) indicate the old implementation was fragile. The point<T> is de-facto an integer-based type and it should fullfil the promised behaviour internally, instead of asking users to remember to take care of rounding. The same point<double> dimension, one user may truncate or floor, another may ceil, leading to different results.

mloskot · 2018-10-22T11:47:47Z

Now, I'll have to deal with the failure of Travis job COMPILER=clang++-5.0 VARIANT=ubsan_undefined - https://travis-ci.org/boostorg/gil/jobs/444470812 - in the toolbox/test/test.cpp.
Things are getting interesting :)

mloskot · 2018-10-22T16:59:21Z

AFAIU, the ubsan_undefined failure is not reporting any issues in GIL, but somewhere else!

====== BEGIN OUTPUT ======
boost/function/function_template.hpp:768:14: runtime error: call to function boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) through pointer to incorrect function type 'void (*)(boost::detail::function::function_buffer &)'
/home/travis/build/boostorg/boost-root/./boost/function/function_template.hpp:116: note: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) defined here
    #0 0x7f985638b92a in boost::function0<void>::operator()() const /home/travis/build/boostorg/boost-root/./boost/function/function_template.hpp:768:14
    #1 0x7f985638b73a in boost::detail::forward::operator()() /home/travis/build/boostorg/boost-root/./boost/test/impl/execution_monitor.ipp:1312:32
    #2 0x7f985638b73a in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /home/travis/build/boostorg/boost-root/./boost/function/function_template.hpp:138
    #3 0x7f9856385b91 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /home/travis/build/boostorg/boost-root/./boost/test/impl/execution_monitor.ipp:875:16
    #4 0x7f9856385df8 in boost::execution_monitor::execute(boost::function<int ()> const&) /home/travis/build/boostorg/boost-root/./boost/test/impl/execution_monitor.ipp:1214:16
    #5 0x7f9856387c0e in boost::execution_monitor::vexecute(boost::function<void ()> const&) /home/travis/build/boostorg/boost-root/./boost/test/impl/execution_monitor.ipp:1321:5
    #6 0x7f98563d2858 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned int) /home/travis/build/boostorg/boost-root/./boost/test/impl/unit_test_monitor.ipp:49:9
    #7 0x7f9856397f44 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned int, boost::unit_test::framework::state::random_generator_helper const*) /home/travis/build/boostorg/boost-root/./boost/test/impl/framework.ipp:789:44
    #8 0x7f98563980f0 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned int, boost::unit_test::framework::state::random_generator_helper const*) /home/travis/build/boostorg/boost-root/./boost/test/impl/framework.ipp:737:54
    #9 0x7f98563980f0 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned int, boost::unit_test::framework::state::random_generator_helper const*) /home/travis/build/boostorg/boost-root/./boost/test/impl/framework.ipp:737:54
    #10 0x7f9856391a41 in boost::unit_test::framework::run(unsigned long, bool) /home/travis/build/boostorg/boost-root/./boost/test/impl/framework.ipp:1631:29
    #11 0x7f98563cf4eb in boost::unit_test::unit_test_main(bool (*)(), int, char**) /home/travis/build/boostorg/boost-root/./boost/test/impl/unit_test_main.ipp:247:9
    #12 0x7f9854eaaf44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
    #13 0x40a4f5 in _start (/home/travis/build/boostorg/boost-root/bin.v2/libs/gil/toolbox/test/test.test/clang-linux-5.0.2/ubsan_undefined/visibility-hidden/test+0x40a4f5)
EXIT STATUS: 1
====== END OUTPUT ======

There is no GIL location at the top of the trace.

Eventually, I've prepared mwe and reported it here boostorg/function#29

…ouble> The free variants of operator/ and operator* now yield new type of point<T> with T promoted to type common for both operands. Add missing member operator*=, for symmetry. Add template function iround(point<T>) as no-op sink for point specialisations for T of integer types. Add explicit casts of arithmetic intermediate results to avoid compilation warnings. Replace redundant point<T> construction in return statement with uniform initialization. Add static_assert validating range of axis_value integer template parameter. Update test/point/point.cpp with basic checks of all point<T> members. Fixes boostorg#153

mloskot · 2018-10-24T07:34:40Z

@stefanseefeld The CI-s are green now. Approve?

mloskot added the cat/enhancement Improvements, but not fixes addressing identified bugs label Oct 21, 2018

mloskot requested review from stefanseefeld and chhenning October 21, 2018 18:36

mloskot added this to In Progress in Trac Tickets Clean-Up Oct 21, 2018

stefanseefeld reviewed Oct 21, 2018

View reviewed changes

include/boost/gil/point.hpp Show resolved Hide resolved

mloskot force-pushed the ml/fix-point-division-op branch from 693bfe3 to f9952e8 Compare October 21, 2018 20:41

mloskot force-pushed the ml/fix-point-division-op branch from f9952e8 to 8d453fc Compare October 22, 2018 13:09

mloskot force-pushed the ml/fix-point-division-op branch from 8d453fc to dcc3d82 Compare October 22, 2018 17:55

mloskot changed the title ~~Fix point<T> divide and multiply to not to fix result to point<double>~~ Fix point<T> divide and multiply to not to hardcode result as point<double> Oct 22, 2018

mloskot force-pushed the ml/fix-point-division-op branch from dcc3d82 to 1534a4d Compare October 23, 2018 20:01

stefanseefeld approved these changes Oct 24, 2018

View reviewed changes

mloskot merged commit a1c966d into boostorg:develop Oct 24, 2018

Trac Tickets Clean-Up automation moved this from In Progress to Done Oct 24, 2018

mloskot deleted the ml/fix-point-division-op branch October 24, 2018 13:09

mloskot mentioned this pull request Oct 24, 2018

operator / for point2 is hard coded to operate on type double (Trac 11739) #153

Closed

mloskot added this to the Boost 1.69+ milestone Oct 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix point<T> divide and multiply to not to hardcode result as point<double> #157

Fix point<T> divide and multiply to not to hardcode result as point<double> #157

mloskot commented Oct 21, 2018 •

edited

mloskot commented Oct 21, 2018 •

edited

stefanseefeld commented Oct 21, 2018

mloskot commented Oct 21, 2018 •

edited

mloskot commented Oct 22, 2018

mloskot commented Oct 22, 2018 •

edited

mloskot commented Oct 24, 2018

Fix point<T> divide and multiply to not to hardcode result as point<double> #157

Fix point<T> divide and multiply to not to hardcode result as point<double> #157

Conversation

mloskot commented Oct 21, 2018 • edited

References

Tasklist

mloskot commented Oct 21, 2018 • edited

stefanseefeld commented Oct 21, 2018

mloskot commented Oct 21, 2018 • edited

mloskot commented Oct 22, 2018

mloskot commented Oct 22, 2018 • edited

mloskot commented Oct 24, 2018

mloskot commented Oct 21, 2018 •

edited

mloskot commented Oct 21, 2018 •

edited

mloskot commented Oct 21, 2018 •

edited

mloskot commented Oct 22, 2018 •

edited