Skip to content
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

Avoid Elemwise fusion for scalar Ops without C implementations #161

Merged

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Nov 15, 2020

This PR fixes an issue involving the fusion of Elemwise Ops with scalar Ops that do not have Op.c_code implementations.

There were no direct tests for the relevant condition in local_elemwise_fusion_op (i.e. the return missing from within here), so, naturally, this issue went unnoticed. It's possible—albeit doubtful—that there were even any indirect tests for this (whether intentional or not).

This is yet another great example of how badly we need to refactor the tests (e.g. #141, #23, etc.) and dramatically change the testing standards. For example, the test added in this PR creates its own Op that lacks a c_code method; had we used an existing Op that lacked a c_code method (e.g. theano.tensor.basic.i0) we would've implicitly added an unnecessary testing dependency/requirement, and, in the somewhat likely event that a c_code implementation is given to said Op, the entire test would've been silently invalidated.
Likewise, the same test attempts to force C compilation and explicitly includes the relevant optimizations; however, many tests in that module simply assume that the optimizations they're testing are enabled in the default mode object (e.g. mode_opt in tests.tensor.test_opt). This approach is confounded by all the other optimizations enabled in the default/global mode, and it makes all the tests that rely on it brittle and highly dependent on those other unrelated optimizations.

Closes #160.

@brandonwillard brandonwillard added bug Something isn't working graph rewriting labels Nov 15, 2020
@brandonwillard brandonwillard self-assigned this Nov 15, 2020
@brandonwillard brandonwillard changed the title Fix fusion c code condition Avoid Elemwise fusion for scalar Ops without C implementations Nov 15, 2020
@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #161 (2de5415) into master (0150ddf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #161   +/-   ##
=======================================
  Coverage   70.42%   70.43%           
=======================================
  Files         163      163           
  Lines       54782    54783    +1     
=======================================
+ Hits        38582    38584    +2     
+ Misses      16200    16199    -1     
Impacted Files Coverage Δ
theano/tensor/opt.py 90.42% <100.00%> (+0.06%) ⬆️
theano/gof/opt.py 67.28% <0.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0150ddf...2de5415. Read the comment docs.

@brandonwillard
Copy link
Member Author

Ha, couldn't have asked for a better example of how problematic these highly coupled tests are: the current testing error is caused by that run having the "fast compile" flag set (see here), which changes the global compilation mode's properties!

When global/default mode is set to FAST_COMPILE many optimizations are disabled, and—in this case—the optimization being tested was disabled, so this test was never actually testing what it intended to test when run in this mode!

We need to completely remove this framework in which we rerun all the tests in different compilation modes; it's a massive waste and it only highlights the poor quality of our tests.

@brandonwillard brandonwillard force-pushed the fix-fusion-c-code-condition branch from 7a55dad to 2de5415 Compare November 15, 2020 02:36
@brandonwillard brandonwillard merged commit 99f08ee into aesara-devs:master Nov 15, 2020
@brandonwillard brandonwillard deleted the fix-fusion-c-code-condition branch November 15, 2020 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working graph rewriting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method not defined I0e.c_code
1 participant