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

Consider changing Elemwise and CAReduce to abstract classes #712

Open
brandonwillard opened this issue Dec 30, 2021 · 0 comments
Open

Consider changing Elemwise and CAReduce to abstract classes #712

brandonwillard opened this issue Dec 30, 2021 · 0 comments
Labels
enhancement New feature or request help wanted Extra attention is needed important question Further information is requested refactor This issue involves refactoring request discussion

Comments

@brandonwillard
Copy link
Member

brandonwillard commented Dec 30, 2021

If we change Elemwise and CAReduce to abstract classes, we can start treating Elemwise + scalar Op combinations as types and more easily distinguish them and generalize their handling in code.

Our Elemwise and CAReduce instances are already acting as singletons, and there isn't a critical use for "dynamically" generated Elemwise instances, so the shift to explicit subclasses won't change much. If anything, it will simplify a lot of our code, because we would be able to use isinstance(op, Add) and cover every Elemwise(add) that we currently need to check with isinstance(op, Elemwise) and op.scalar_op == add and the like. It would also allow us to fix some really poor logic like the type(node.op) == CAReduce used here, and the brittle, convoluted type checking used here.

The changes involved in this mostly involve replacing/changing the use of scalar_elemwise. Instead, where scalar_elemwise is currently used to create an Elemwise instance, we would create an Elemwise subclass and its single instance.

@brandonwillard brandonwillard added enhancement New feature or request help wanted Extra attention is needed question Further information is requested important refactor This issue involves refactoring request discussion labels Dec 30, 2021
@brandonwillard brandonwillard added this to the Clean up after Theano milestone Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed important question Further information is requested refactor This issue involves refactoring request discussion
Projects
None yet
Development

No branches or pull requests

1 participant