Make Canonizer always collapse nested ops

[brandonwillard]

Fixes #6685

[twiecki]

LGTM. I think a more specific test like #6685 would be good.

[brandonwillard]

@twiecki, the uncommented lines in

Theano/theano/tensor/tests/test_opt.py

Line 370 in 5b06db2

(fx + fy + fz, (fx, fy, fz), (fxv, fyv, fzv), 1, 'float32'),

and

Theano/theano/tensor/tests/test_opt.py

Line 372 in 5b06db2

(fx * fy * fz, (fx, fy, fz), (fxv, fyv, fzv), 1, 'float32'),

are effectively the same—previously—non-collapsing x + y + z setup as my example in #6685. The assert in

Theano/theano/tensor/tests/test_opt.py

Line 427 in 5b06db2

assert(len(f.maker.fgraph.toposort()) == nb_elemwise)

checks that there's only one Apply node corresponding to the single, collapsed add/mul, which is close to what I would've done within the context of #6685.

Otherwise, I agree with the idea of being more direct by performing the opts exclusively under tt.opt.local_add_canonizer/tt.opt.local_mul_canonizer and without the other implicit opts or function machinery. Such a set of tests would—for instance—likely require considerably less runtime and avoid unwanted interactions with other opts and function compilation processes. However, without changing all the relevant tests to work in such a way, I doubt those improvements would really be realized.

My only hesitation involves those interactions and the general question "Besides the clearly relevant Apply nodes check, what exactly are those tests targeting?" Does it include the non-opt aspects of function compilation? The compiled graph is being used for a single numerical calculation, but all that's checked is the resulting dtype; why not optimize the FunctionGraph and simply check fgraph.outputs[0].dtype? Alternatively, wouldn't a test value accomplish the same thing?

If the consensus is that those opt interactions shouldn't be there (or at least tested there) and that we don't need a function-derived numerical dtype check, then I'll simply refactor those tests to more directly target the relevant opts and graph changes alongside—if necessary—some simple test value checks.

[brandonwillard]

By the way, with these changes, the currently disabled test_canonize.test_elemwise_multiple_inputs_optimisation2 succeeds (after fixing a dtype misspecification, e.g. by adding allow_input_downcast=True). There's considerable overlap between
that set of tests and the currently active test_canonize.test_elemwise_multiple_inputs_optimisation, but the former does add DimShuffleed vector cases.

[twiecki]

I'll wait for @nouiz to take a look also.

[nouiz]

I'm reluctant to change that in the current status of Theano (dead).
You are right that you possibly fix some long standing issue in Theano.
But as this is a long standing issue in Theano, some optimization could rely on this behavior.
So there could be other optimization that need to be adapted to this behavior.

If Theano was still actively developed, I would tell we should do it.
But introducing potential degradation inside Theano when there isn't good support to help diagnose and fix possibly consequence that Ididn't foresaw isn't something that I find interesting.

Now, if this change was optional and not enabled by default, I would be happy to review the code in detail and merge it when ready.

To make it optional, you can check in the file theano/configdefaults.py and add a Theano flags for it.

[brandonwillard]

As long as the project isn't archived and there are people willing to submit, review and merge PRs, it's still alive! That said, I can understand if you're saying that you personally don't want to review PRs; that's perfectly fine. Otherwise, why not archive the repo?

Likewise, I'm not clear on the stated concerns: is the current test suite insufficient for validating these changes? Is the concern for users and external libraries that rely on master?

In light of the aforementioned concerns, adding more code and potential points of failure in lieu of fixing an existing—albeit broken—functionality contract, doesn't sound good.

[nouiz]

I'm fine doing review. This is what I'm doing. But there isn't significant work. Very minimal work is being done in Theano. So I review code with that point of view.

The current test could find some possible bad consequence of this PR. But it certainly can't guaranty there isn't. We do not have a speed benchmark for example.

What do you think of making this change optional?