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

Non-shortcircuiting if_else should be default for MX #1968

Closed
jaeandersson opened this issue Mar 7, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@jaeandersson
Copy link
Member

commented Mar 7, 2017

The shortcircuiting if_else for MX is error prone. I think we should change the default behavior to be non-shortcircuiting, just like for SX. Then, if you really want short-circuiting, you have to specify this. Or work with Function instead (which might be much cleaner).

Cf. #1967

from casadi import *
c = SX.sym('c')
x = SX.sym('x')
y = SX.sym('y')
print(if_else(c, x, y)) # ((c?x:0)+((!c)?y:0))
c = MX.sym('c')
x = MX.sym('x')
y = MX.sym('y')
print(if_else(c, x, y, True)) # switch(c, x, y){0}
print(if_else(c, x, y, False)) # ((c?x:0)+((!c)?y:0))
print(if_else(c, x, y)) # switch(c, x, y){0}   <=== change this

@jaeandersson jaeandersson self-assigned this Mar 7, 2017

@jaeandersson

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2017

@ghorn @jgillis Agreed?

@ghorn

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

Is the error-proneness is orthogonal to the decision?

@jaeandersson

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2017

Is the error-proneness is orthogonal to the decision?

Currently, the default behavior for MX (but not SX), is that when it encounters an expression such as

if_else(c, <some expression>, <some expression>)

to identify the primitives in the expressions and construct functions that are passed to the Switch node. This can lead to unnecessary recalculations in very simple cases, like:

x = <some very large and wieldy expression>
y = x + 1;
if_else(c, x, y)

It's better that the default for MX is the same as the default for SX, which is also less error prone.

@jgillis

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

I have no opinion of this.
If your model needs switches, you must be doing it wrong he;-)

@jaeandersson

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2017

OK, I'll go ahead with this then. Not sure if we need a soft deprecation for this... maybe not.

@jaeandersson jaeandersson added this to the Version 3.2 milestone Mar 30, 2017

jaeandersson added a commit that referenced this issue Mar 30, 2017

jaeandersson added a commit that referenced this issue Mar 30, 2017

jaeandersson added a commit that referenced this issue Mar 30, 2017

jaeandersson added a commit that referenced this issue Mar 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.