-
Notifications
You must be signed in to change notification settings - Fork 74.2k
-
Notifications
You must be signed in to change notification settings - Fork 74.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tf.copy() as alternative to tf.identity() #11186
Comments
Just curious, could you elaborate what kind of bugs it could lead to? |
Couldn't we use |
@ppwwyyxx For example, I was trying to feed the transition tuple of state, action, reward, and successor state to a reinforcement learning agent. Here, # NOTE: old_state actually refers to the new state after stepping the env. Need env.state * 1 instead.
old_state = tf.identity(env.state)
with tf.control_dependency([old_state]):
action = agent.act(env.state)
reward = env.step(action)
with tf.control_dependency([reward]):
experience_op = agent.experience(old_state, action, reward, env.state) @jyegerlehner That would require creating a second variable to hold the old state of the variable. That's wasteful if the value is only needed inside each |
I believe your issue can be described in this small repro: ones = np.ones((10,10))
v = tf.get_variable('asdf', shape=[10,10], dtype=tf.float32)
old_state = v.read_value() # equivalent to tf.identity
with tf.control_dependencies([old_state]):
reward = tf.assign_add(v, ones)
with tf.Session() as sess:
tf.global_variables_initializer().run()
a, b = sess.run([reward, old_state])
print(a.sum(), b.sum()) The above code, though appears to be deterministic (due to the control dependency), actually is not. It sometimes prints two equal results, sometimes not. Using |
I also wonder if we could presumably apply tf.copy to tf.Variable, tf.placeholder and so on. |
@thjashin And my same solution seems applicable. |
@ppwwyyxx Yeah using |
@danijar if var and var_identity are in the same device, var_identity would be similar to a C++ reference to var (basically they are the same thing in the same memory, var_identity is just an alias for var), any change in var would be represented equally in var_identity. In this sense, I think implementation-wise, it is similar to that in get_variable() function set var_identity a reusable variable of var. if var_identity is transferred to another device, which I think it is pretty typical when training multiple workers and they are shared by one param server, the var_indentity is essentially a copy of var. This makes sense, because, between different devices, nothing can be really shared but a copy would be the best solution. |
Based on above snippet, I think tf.identity() works as deep copy. as least in tensorflow 1.11.0. |
As demonstrated by @chuchienshu above, |
@dynamicwebpaige This issue was not about whether or not |
Remember tensor2 is a Variant. You may use take(-1) in case tensor1 is a Dataset and you want tensor2 to be one of the Dataset family only. |
tf.identity(tensor)
does or does not create a copy of the tensor based on whether it's on the same device. This can lead to bugs that are hard to find. The current way of ensuring a copy is to perform an arithmetic/logic operation that doesn't change the value, such astensor + 0
,1 * tensor
, ortf.equal(tensor, True)
. Needless to say, this makes code hard to read. Moreover, different treatment is needed for different tensor types. Can we have atf.copy(tensor)
that does this for us?The text was updated successfully, but these errors were encountered: