Bug with double reshape optimization about theano HOT 11 CLOSED

So, to answer my own question, I think it's a good idea to try to guess broadcastability, as it should not be harmful (although, out of curiosity, I ran the test suite with this feature disabled and all tests pass except the one that specifically test for this feature).

Here is a suggestion for a better fix (better than #284):

1. Add an optional broadcastable argument to Reshape.make_node. When this argument is provided, then the op just uses the provided broadcastable pattern for the output, without attempting to make any guess. The local_reshape_chain optimization would then use this argument to force the broadcastable pattern to remain unchanged.
2. In a normal situation, we should expect the current implementation of local_reshape_chain not to modify the broadcastable pattern anyway. If it modifies it, it means there is probably something suspicious elsewhere. So I would also suggest to have a warning when this happens (which could be disabled by a config.warn option).

Thoughts?

from theano.

I merged #284 as this fix an issue. This will help find the root cause in the problem in issue #276.

I still don't understand why this error happen. But if we don't care about that, I would prefer that we add a rebroadcast op after the new reshape then allow Reshape.make_node take an additional parameter. That is what we did at a few other place where it make sense.

If I have time, I'll try to understandwhy the broadcast pattern change.

from theano.

Are Rebroadcast Ops automatically removed during the optimization phase? If yes then I'm good with it.

from theano.

Not yet, but they should be we should do it. Anyway, they are inplace, so they are a fast constant time for now.

from theano.

I created gh-288 issue for to remove the rebroadcast

from theano.

My main concern is that Rebroadcast Ops may disrupt some optimizations (because the pattern that an optimization is looking for will typically not contain Rebroadcasts).
It seems to me that whenever we have a Rebroadcast which is the only parent of its input, then we can remove the Rebroadcast (within the optimization process) and just modify the type of the input directly. Am I wrong?

from theano.

It is not that easy. If you try to "clone the graph"/"rebuild the graph with other inputs" without the Rebroadcast, it could have problem. Maybe we could work around that and allow this, but this is not an easy/fast change. The real underling questions is how to remove it automatically for all op without making the graph being able to be rebuild.

Or maybe we can just continue in the direction that optimized graph are not portable and tell they are not rebuildable? That should way after 0.5 and I would prefer confirmation by other people on that before doing it.

from theano.

Ok.
What if we keep the type of each variable in the graph as part of the graph information needed to "clone / rebuild" the graph? (not sure if that's a sensible idea -- I don't know where in the code we do this kind of operation)

from theano.

I think we should continue that discussion after NIPS. I have other stuff to think about and this is not easy change to do if we don't want to break stuff. But as I told, maybe the final solution is to don't care about clone/rebuild as this is during compilation.

from theano.

No problem, this can wait.

from theano.

There is a ticket for the general fix: gh-288, so I close this one.

from theano.