search

Enny1991 / PLSTM

137 stars 32 forks source link

TensorFlow v1.0 compatibility fixes #9

Closed tomrunia closed 7 years ago

tomrunia commented 7 years ago

Changes to make the PhasedLSTMCell compatible with TensorFlow v1.0.

tomrunia commented 7 years ago

Don't merge this yet. It seems like there is a problem, most likely with the tf.mod and the @ops.RegisterGradient("FloorMod").

Enny1991 commented 7 years ago

Yeah I have been converting all my work in these days to v1.0, I'll also work on that. Keep you posted. Thanks

+Enea

Enny1991 commented 7 years ago

Should be working now for v1.0. I had to create another file just to make sure it can still run for 0.x versions.

Let me know if it works for you. +Enea

tomrunia commented 7 years ago

Hi Enny, thanks for the commits! It seems like your code is running fine. However, I have a question about the code. What is the reason you evaluate phi_t with a double tf.mod? The result of the following two statements looks pretty identical to me:

# Your implementation
phi = tf.div(tf.mod(
    tf.mod(times - s_broadcast, tau_broadcast) + tau_broadcast, tau_broadcast), tau_broadcast
)

# Alternative
phi = tf.divide(tf.mod(times - s_broadcast, tau_broadcast), tau_broadcast)
Enny1991 commented 7 years ago

Hey @tomrunia So I had some past experience with some other framework where the mod for fractional and negative number was wrongly defined. And I had to use this trick to make it correct. I have actually never checked if it's the case for tensorflow. But if the definition of mod is correct then the 2 are identical. I should probably check and make the code lighter in case:)

Thanks for pointing that out. +Enea