Closed amilsted closed 4 years ago
I can't see how this is helpful. Edge
and Node
already point to their parent network, and we already throw an error if the networks are mismatched.
require defining a global variable
No! Bad ash.
Tensorflow did something similar with their tf.Graph
objevt, and they are gutting that feature in TF 2.0 because it confused the hell out of users and caused code to break in really subtle ways.
I can't see how this is helpful.
Edge
andNode
already point to their parent network, and we already throw an error if the networks are mismatched.
Indeed, it would just be a different syntax for the same thing. The idea was just to clarify the context-dependency (on the parent network).
Tensorflow did something similar with their
tf.Graph
objevt, and they are gutting that feature in TF 2.0 because it confused the hell out of users and caused code to break in really subtle ways.
Well, my thought was to have this global only set (and unset) by the TensorNetwork
context manager. If an expression with @
or ^
were used outside such a context, this would still be an error. Probably one would want to make nesting such contexts an error also.
Anyway, I recognize I am probably just being a little paranoid here, so if nobody else agrees there is a problem, I promise I will try to forget about it :)
Let's revisit it in 2 weeks. If you can find a code sample that doesn't give a clear error message already and can be fixed with the context manager, then we'll implement it. Sound good?
There's a nagging worry in my head that our overloading of
@
and^
might cause people problems, since these operations involve not onlyNode
s orEdge
s - they also modify the parentTensorNetwork
. I can't think of another case where operators work like this.Perhaps we should make it necessary to use these within a context, to make this clearer? For example:
Apart from providing
__enter__
and__exit__
methods forTensorNetwork
, AFAICT this would require defining a global variable to hold the "current network", but I don't see an obvious problem with that.