vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
1.14k stars 136 forks source link

PolyRatFun performance regression #178

Closed tueda closed 7 years ago

tueda commented 7 years ago

We have observed a considerable performance regression on PolyRatFun computations, introduced by ffabb37. For now, difficulty of fixing this regression is unclear.

jodavies commented 7 years ago

For me, the commits since 5634f180c4a508055933b9cecdd18ce94c6a6e2c break some code which uses polyratfun. I get

Division by zero during normalization

I am at a conference so do not have time to bisect the problem for a few days or to produce a minimal example, but I will get back to you...

Josh.

vermaseren commented 7 years ago

Hi Josh,

In this case the date of the sources is rather important. We had a similar problem two or three days ago, but that has been repaired to my knowledge. We tried to repair some internal inefficiencies in the application of polyratfun. This involved using some already existing routines, but it turned out they were not exactly doing what we thought they should be doing (they are part of the original rational polynomial code by Jan). It turned out a bit more complicated than thought, but we did not want to go back to the older slower code, because in some cases the regression was enormous.

If you have a crashing example with the latest commit, I would like to get that. What happened had (in our case) to do with PolyRatFun rat,RAT; and the use of the RAT function. In the worst case we also have an elegant workaround for that.

Why did we push? It all worked on my examples and nontrivial test runs. It also passed all the tests in the make check. Then we got the crash eventually in an example of Takahiro. This was repaired on monday (3 april). Since then I have been trying to tune the performance a bit more.

This should explain what happened.

Jos

On 5 apr. 2017, at 00:51, Josh Davies notifications@github.com wrote:

For me, the commits since 5634f18 https://github.com/vermaseren/form/commit/5634f180c4a508055933b9cecdd18ce94c6a6e2c break some code which uses polyratfun. I get

Division by zero during normalization I am at a conference so do not have time to bisect the problem for a few days or to produce a minimal example, but I will get back to you...

Josh.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/178#issuecomment-291662395, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEio6TM2EUXXYjnU0ACAVlSLtwzbPks5rssljgaJpZM4Mug5A.

jodavies commented 7 years ago

OK, I managed to produce a minimal example which demonstrates the crash,

#-
Off Statistics;

Symbol a,b,c,ep;
CFunction redprf,epprf;

Local test1 =
       + epprf(-1, - 1 + ep)*redprf(1,1)
       + epprf(-1,1 - 3*ep + 2*ep^2)*redprf(-1,1)
      ;
.sort

PolyRatFun redprf;
Identify redprf(a?,b?) = redprf(a*c,b*c);
Identify epprf(a?,b?) = redprf(a,b);
.sort

Print +s;
.end

This works OK with 5634f180c4a508055933b9cecdd18ce94c6a6e2c but gives the division by zero error with 5b55fdb4b19e2a00aa9e2e76a86f623c8ca1c178 .

Interestingly, if I comment the .sort after the Identify statements, it prints

   test1 =
       + redprf( - 2 + 4*ep - 2*ep^2,0)
      ;

with the bad commit.

Thanks, Josh.

EDIT: this is introduced by a0b635c6ba559227abc4bd7cdbbfe272af4c2941

vermaseren commented 7 years ago

Hi Josh,

Thanks! That helped. I found the problem and put the repair in the github.

Cheers

Jos

On 5 apr. 2017, at 16:25, Josh Davies notifications@github.com wrote:

OK, I managed to produce a minimal example which demonstrates the crash,

-

Off Statistics;

Symbol a,b,c,ep; CFunction redprf,epprf;

Local test1 =

  • epprf(-1, - 1 + ep)*redprf(1,1)
  • epprf(-1,1 - 3ep + 2ep^2)*redprf(-1,1) ; .sort

PolyRatFun redprf; Identify redprf(a?,b?) = redprf(ac,bc); Identify epprf(a?,b?) = redprf(a,b); .sort

Print +s; .end This works OK with 5634f18 https://github.com/vermaseren/form/commit/5634f180c4a508055933b9cecdd18ce94c6a6e2c but gives the division by zero error with 5b55fdb https://github.com/vermaseren/form/commit/5b55fdb4b19e2a00aa9e2e76a86f623c8ca1c178 .

Interestingly, if I comment the .sort after the Identify statements, it prints

test1 =

  • redprf( - 2 + 4ep - 2ep^2,0) ; with the bad commit.

Thanks, Josh.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vermaseren/form/issues/178#issuecomment-291877968, or mute the thread https://github.com/notifications/unsubscribe-auth/AFLxEiLYdrWlgAhmHS1pQSqwi3j0XJfRks5rs6RVgaJpZM4Mug5A.

tueda commented 7 years ago

Perhaps now we can turn off this performance regression alert.