CyberZHG/keras-transformer-xl

Memory-Test does not evaluate a single model

xuevin opened this issue · 4 comments

Describe the Bug

I believe that test is intended to evaluate the Memory function for 10 batches of sentences (or the same sentence for 10 epochs).

The current implementation initializes the Model 10x instead of initializing one model and evaluating 10 batches.

Version Info

  • I'm using the latest version

Minimal Codes To Reproduce
NA

I think the recommended change is to shift the model instantiation outside of the for loop.

        for _ in range(10):
            input_data = keras.layers.Input(shape=(3, 3))
            input_length = keras.layers.Input(shape=(1,))
            output = Memory(3, 5, 3, 3)([input_data, input_length])
            model = keras.models.Model(
                inputs=[input_data, input_length],
                outputs=output,
            )
            ....
        input_data = keras.layers.Input(shape=(3, 3))
        input_length = keras.layers.Input(shape=(1,))
        output = Memory(3, 5, 3, 3)([input_data, input_length])
        model = keras.models.Model(
            inputs=[input_data, input_length],
            outputs=output,
        )
        for _ in range(10):
            ...

No. This is intend to make sure the order of calculation is correct.

See Use more memory to avoid ordering issue

I am not following why the nested function needs to be executed 10x. Given that you are calling Memory inside the for loop, it reinitializes the state, such that the output will always be the same.

My comment is regarding why

for _ in range(10): exists

Because before that commit, the memory block produces wrong output with a random probability. I have to make sure it always produces the expected output.

Thanks for clarifying. I appreciate your work!