Migrate dense image warp

[WindQAQ]

Address #25. CI test passed on my local machine. Note that the test of _interpolate_bilinear in dense_image_warp will fail if I use the decorator tf.function on it (that's why I haven't added it yet). I'm still trying to figure out why.

[facaiy]

Thanks for the PR :-)

[facaiy]

@WindQAQ Tzu-Wei, don't forget to update README.md https://github.com/tensorflow/addons#contents

[facaiy]

Very close! Good job!

[facaiy]

@WindQAQ Good job! Could you merge the latest master branch? I'd like to merge the PR when the tf.function problem is solved.

[WindQAQ]

Done :-)

[facaiy]

@alextp @tomerk Hi, any update? Could you confirm what's wrong here? bug, or use it incorrectly? Thank you

[alextp]

@tomerk it's weird because assert_greater_equal and assert_equal both boil down to Assert which is marked as stateful. assert_equal though has a lot more logic to try to evaluate the assert at graph build time, so we might not be triggering it at run time at all here.

The automatic control dependencies for assert only ensure the asserts run in order, not that assert is a hard barrier between all computation before and all computation after (we can change this). Maybe this is what's triggering the problem here?

[tomerk]

Okay I now had a chance to actually look at the test case. This test case is broken because it never executes the function in graph mode, it just constructs it in the graph. If you change the function to return a value and evaluate it in both eager & graph it passes.

import tensorflow as tf
from tensorflow.python.framework import test_util


@tf.function
def value_assert(x):
    tf.debugging.assert_greater_equal(
        x, 2, message="Not greater or equal than 2.")
    return x


class AssertTest(tf.test.TestCase):

    @test_util.run_in_graph_and_eager_modes
    def test_value_assert(self):
        with self.assertRaisesRegexp(tf.errors.InvalidArgumentError,
                                     "Not greater or equal than 2."):
            self.evaluate(value_assert(tf.constant(1)))


if __name__ == "__main__":
    tf.test.main()

It will still have the distracting warning about should_use when running in functions inside TF 1.x graph mode, but it won't cause the test to fail.

@alextp as long as the asserts remain ordered w/ all stateful ops it might be okay to not have the asserts be dependencies of all future computation. It could cause some ops to raise other errors before the assert has a chance to run though.

[WindQAQ]

@tomerk Thanks for the workaround here!

It could cause some ops to raise other errors before the assert has a chance to run though.

I think that most of user cases might involve this kind of situation such as the following script (and the test case in this PR):

import tensorflow as tf
from tensorflow.python.framework import test_util


@tf.function
def foo(x):
    tf.debugging.assert_greater_equal(x.shape[0], 2, message="fail")
    y = x[1]
    return y


class TestAssert(tf.test.TestCase):

    @test_util.run_in_graph_and_eager_modes
    def test_assert(self):
        with self.assertRaisesRegexp(tf.errors.InvalidArgumentError, "fail"):
            self.evaluate(foo(tf.random.uniform(shape=(1, 2, 3))))


if __name__ == "__main__":
    tf.test.main()

[tomerk]

Yeah, it's definitely not ideal because a less useful error message would get thrown in these cases (so we should throw it on the to-do list). Fortunately I don't think we're at a big risk of silent errors where code finishes executing without errors when it should have failed.

[seanpmorgan]

Thanks for the great work. If you could just update interpolate_bilinear as requested and fix the lint error in the CI this LGTM.

[WindQAQ]

@seanpmorgan Made requested changes Thanks 😄

[WindQAQ]

Sorry that I forget to merge latest commits. Anyway, @seanpmorgan, now CI check should pass.

[seanpmorgan]

Thank you very much!