-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update to TF2 #100
Update to TF2 #100
Conversation
We still disable eager execution, but now everything else uses TF2 behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just leave two trivial questions.
mloop/neuralnet.py
Outdated
name="weight_"+str(i))) | ||
biases.append(tf.Variable( | ||
tf.random_normal([dim], stddev=bias_stddev), | ||
bias_initializer(shape=[1], dtype=tf.float32), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess 1 also works here, but any reason we want to use the same value for all nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, well spotted!
self.keep_prob_placeholder = tf.placeholder_with_default(1., shape=[]) | ||
self.regularisation_coefficient_placeholder = tf.placeholder_with_default(0., shape=[]) | ||
self.input_placeholder = tf.compat.v1.placeholder( | ||
tf.float32, shape=[None, self.num_params]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a question, would this inconsistency in dtype cause any trouble if calling NN from qctrl-mloop since we are using 64 there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there'll be any issues, because the M-LOOP neural net learner never needs to talk to the qctrl-mloop
learners (even if they did get used together, there are separate graph objects involved, so I don't think we'd ever be mixing tensors from one to the other).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and as for any inconsistency when converting between NumPy data and tensors, I guess it could lead to some loss of precision (e.g. if the user is using float64 for costs and parameters), but I don't think it will be a big deal. I think the double precision is mainly useful for the internals of computations involving messy operations like division and whatnot; for the beautiful and smooth things we use in the neural network I can't imagine any issues arising.
Still disabling eager execution, but at least we're not disabling all TF2 behaviour.
Related to #37.