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

Make ifelse use Switch when its arguments are ScalarTypeed #1226

Open
brandonwillard opened this issue Sep 30, 2022 · 4 comments
Open

Make ifelse use Switch when its arguments are ScalarTypeed #1226

brandonwillard opened this issue Sep 30, 2022 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed performance concern

Comments

@brandonwillard
Copy link
Member

brandonwillard commented Sep 30, 2022

Since Switch is quite literally the scalar version of IfElse—at least in terms of its Op.c_code implementation, we should consider making invocations of the ifelse helper function return Switches when its arguments are ScalarTypeed.

This should increase the chances that the resulting Op can/will be fused with other scalar Ops to produce a single, more efficient Composite Op, as well as remove some minor overhead due to TensorFromScalar and ScalarFromTensor.

The same general idea can be applied through rewrites, although we'll probably get a lot of coverage using only changes like this at the interface-level. In cases where TensorFromScalar arguments are passed to ifelse, an undesirable IfElse Op would be used, and this is where a rewrite could be helpful.

Originally posted by @brandonwillard in #1217 (comment)

@brandonwillard brandonwillard added good first issue Good for newcomers help wanted Extra attention is needed enhancement New feature or request performance concern labels Sep 30, 2022
@jessegrabowski
Copy link
Contributor

I'm taking a look at this. How should the case of list-like inupts to then/else conditions of ifelse be handled? aesara.scalar.switch doesn't seem to allow lists in the same way ifelse does.

@brandonwillard
Copy link
Member Author

brandonwillard commented Oct 4, 2022

I'm taking a look at this. How should the case of list-like inupts to then/else conditions of ifelse be handled? aesara.scalar.switch doesn't seem to allow lists in the same way ifelse does.

Good question. We can always nest Switches, but that doesn't sound great, especially not for the readability of our graphs.

N.B. This reminds me of a couple scalars-related issues: there are almost no scalar rewrites that simplify these basic operations, and Composites can contain other Composites. Both of these can lead to unnecessarily large scalar sub-graphs (e.g. see the scalar sub-graphs constructed by some of the Scan tests). See #1235.

@jessegrabowski
Copy link
Contributor

A list of Switchs is the natural answer, but I agree it's not very beautiful. An alternative could be a full-blown scalar version of ifelse added to the scalar module? If there is enough of a distinction to merit two separate functions Switch and IfElse in the tensor case, does it then follow that there's enough distinction for the scalar case as well?

@brandonwillard
Copy link
Member Author

A list of Switchs is the natural answer, but I agree it's not very beautiful. An alternative could be a full-blown scalar version of ifelse added to the scalar module? If there is enough of a distinction to merit two separate functions Switch and IfElse in the tensor case, does it then follow that there's enough distinction for the scalar case as well?

We should be fine with nested Switches for now, especially since we could rewrite some of them as single Composites (e.g. via #1225).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed performance concern
Projects
None yet
Development

No branches or pull requests

2 participants