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 roadmap #52

Merged
merged 4 commits into from Aug 8, 2016

Conversation

Projects
None yet
3 participants
@geoffreylitt
Copy link
Contributor

geoffreylitt commented Aug 7, 2016

Just some minor tweaks to the Readme and Roadmap, to make it slightly more accessible for new users/contributors. The only really substantive change is adding support for default graphs to the roadmap.

Let me know if you think these changes make sense @Arafatk! Also I would be happy to work on default graph support if you agree that that is worth having. (Technically that's necessary for the basic usage tutorial, and I think it's a pretty important convenience for new users)

Arafatk and others added some commits Aug 7, 2016

update roadmap
* note completed tasks
* add default graph support to roadmap
* tweaks to Readme readability

@Arafatk Arafatk force-pushed the somaticio:master branch from c72653c to 2e9c711 Aug 8, 2016

@Arafatk Arafatk merged commit 391a94a into somaticio:master Aug 8, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@Arafatk

This comment has been minimized.

Copy link
Collaborator

Arafatk commented Aug 8, 2016

@geoffreylitt Thanks for this.
It would be really nice to have default graphs. If you wish to work on it, that would be good.

@geoffreylitt

This comment has been minimized.

Copy link
Contributor

geoffreylitt commented Aug 15, 2016

@Arafatk I'm thinking an interface that's something like this below, making variable available as some sort of global method in addition to just on a graph instance. It would be great to know if you had other thoughts before I get too far implementing.

var1 = Tensorflow::Variable.new([1, 2, 3], dtype: :int32) # assigns to the default graph

The Tensorflow::Variable.new part feels a bit ugly compared to the python tf.variable, another approach might be a singleton object of some kind.

tf = Tensorflow::Coordinator.new  # there is probably a better name for this singleton
var1 = tf.variable([1, 2, 3], dtype: :int32)

Ruby blocks could also easily support assigning a default graph:

graph2 = Tensorflow::Graph.new
graph2.as_default do
  var1 = Tensorflow::Variable.new([1, 2, 3], dtype: :int32) 
end
@Arafatk

This comment has been minimized.

Copy link
Collaborator

Arafatk commented Aug 15, 2016

@geoffreylitt
Among the two approaches, I think that having a graph as a global method in addition to having an on graph instance would be the very appropriate and easy to use and obviously it must have support to work easily with placeholder, variable and constant.
Thanks for this.

@chrhansen

This comment has been minimized.

Copy link
Collaborator

chrhansen commented Aug 15, 2016

@geoffreylitt I also think that would be a great idea to have default graph and eventually also default session. As @Arafatk says, there's a similar behavior in constant and placeholder.

Note how they in the Python api for Variable https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/ops/variables.py#L30, call get_default_graph(), here https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/ops/variables.py#L337, to fetch from here which is then fetched from here: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/framework/ops.py#L3719.

This and even more so the tf.Session (that automatically assumes the default graph), would also be useful in specs and other places where we constantly have to write 3-4 lines just to evaluate a tensor.

@geoffreylitt

This comment has been minimized.

Copy link
Contributor

geoffreylitt commented Aug 15, 2016

Thanks for the responses guys. Good point re: also supporting placeholder/constant/etc. I was thinking of a very similar implementation using a thread variable.

I'm not quite clear which of the two proposed interfaces you guys prefer and would love to get a temperature read.

Option A: Use constructors on Tensorflow classes

var1 = Tensorflow::Variable.new([1, 2, 3], dtype: :int32)
placeholder = Tensorflow::Placeholder.new([2, 2])

Option B: Have an object with helper methods defined on it

tf = Tensorflow::Coordinator.new  # there is probably a better name for this singleton
var1 = tf.variable([1, 2, 3], dtype: :int32)
placeholder = tf.placeholder([2, 2])

To me, Option A feels slightly more like what one might expect from standard Ruby, but Option B achieves closer similarity to the feel of the Python library, which seems important. Interestingly, Option B actually isn't that different from just creating a graph instance, so I'm starting to question how useful it would really be.

@chrhansen

This comment has been minimized.

Copy link
Collaborator

chrhansen commented Aug 15, 2016

@geoffreylitt I also think Option A would look more familiar to Ruby'ers. If we're consistent in how we translate the python API to Ruby, users should more easy be able to understand the Ruby API. Option A could also be we written with convenience class-methods so we don't have to write .new, and that should make it a bit shorter and bit closer to the Python API. e.g

module CrazyTensorFlowCoordinator
  def self.variable(value, dtype: nil)
    Variable.new(value, dtype: dtype)
  end
end

or whatever you think is the cleanest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment