search

kroneckerbio / kroneckerbio

KroneckerBio modelling toolbox for systems biology
Other
16 stars 10 forks source link

Remove simplifyFraction in diff_vectorized #71

Closed drhagen closed 9 years ago

drhagen commented 9 years ago

Obviously, I don't want to remove this if this speeds up some people's models. But right now, every time I merge into my production folder, I have to go into diff_vectorized and delete the calls to simplifyFraction. These calls cause my model to not build because the "simplification" inflates some expressions that are like 1000 characters long and inflates them above a 1 000 000 characters.

Here is an example that demonstrates the problem, but has been simplified to the point where it won't crash your computer. It's a big expression, but a reasonable one for the ODE of a state in a biological model.

x = 'x_1x';
rate = 'k_46x*x_10x - k_34x + k_32x/((k_4x*k_23x*k_33x*u_5x*x_1x*x_2x*((k_25x*u_4x^2)/(u_4x^2 + 1/4) + 1))/(k_1x*k_2x*k_3x*x_4x) + 1) - (k_41x*x_1x)/k_1x - k_35x*k_46x*x_10x + (k_11x*x_7x)/(k_42x + x_7x) - (k_7x*x_1x)/(k_1x*(k_8x + x_1x/k_1x)) - (k_9x*x_1x)/(k_1x*(k_10x + x_1x/k_1x)) - (k_36x*x_1x)/(k_1x*(k_37x + x_1x/k_1x)) - (k_38x*x_1x*x_3x^2)/(k_1x*(k_39x + x_1x/k_1x)*(x_3x^2 + k_40x^2/(k_23x^2*u_5x^2*((k_25x*u_4x^2)/(u_4x^2 + 1/4) + 1)^2)))';
size(char(diff(rate, x))) % 687
size(char(simplifyFraction(diff(rate, x)))) % 1394285

I have tried all the other simplification functions in Mupad, all of them do this. I think it does a lot of expansion. Mathematical simplification is not the same as computational simplification. Has anyone found that this actually makes their models run faster?

If no one has actually found that this improves things, I propose that we remove simplifyFraction until we find a model that it improves. If this has actually been found to improve a model, then we need to put this behind a flag and debate what the default should be.

dflowers7 commented 9 years ago

If my model builds without simplification, I'm fine with the removal. Otherwise, a simplification flag is fine. I'm out of town without a computer right now, so I won't be able to try this until Monday the 2.

dflowers7 commented 9 years ago

Simplification doesn't seem to make much of a difference in how long second-order symbolic differentiation takes for my model, so let's just remove it for now. If we find a reason that we need it later, we can add a flag that can be set to enable simplification.

xpspectre commented 9 years ago

I'm in favor of keeping this as a flag, with default = false. The documentation should just note that most models build faster and don't run faster with it false.

On Mon, Nov 2, 2015 at 7:35 PM, dflowers7 notifications@github.com wrote:

Simplification doesn't seem to make much of a difference in how long second-order symbolic differentiation takes for my model, so let's just remove it for now. If we find a reason that we need it later, we can add a flag that can be set to enable simplification.

— Reply to this email directly or view it on GitHub https://github.com/kroneckerbio/kroneckerbio/issues/71#issuecomment-153204170 .