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

gcd_ gives wrong results #258

Closed
spj101 opened this issue Jan 18, 2018 · 6 comments
Closed

gcd_ gives wrong results #258

spj101 opened this issue Jan 18, 2018 · 6 comments
Labels
bug Something isn't working

Comments

@spj101
Copy link
Contributor

spj101 commented Jan 18, 2018

It appears that when the gcd_ function is called on certain multi-variate expressions containing terms with rational coefficients the wrong result is given. The error depends on the order in which the symbols appearing in the expressions are defined.

Tested with form and tform on 26793e4 with macOS 10.13.2 and Linux (openSUSE Tumbleweed 20180116).

Example:

#-
Off Statistics;

*Symbols m,s,t; * works
Symbols s,t,m; * fails

L test1= 1/5*s + 1/5*(s+t)*m;
L test2= (s+t)*m;
L result = gcd_(test1,test2); * expect result = 1;

print;
.end

Output:

FORM 4.2.0 (Dec 15 2017, v4.2.0-29-g26793e4) 64-bits  Run: Thu Jan 18 16:50:33 2018
    #-

   test1 =
      1/5*t*m + 1/5*s + 1/5*s*m;

   test2 =
      t*m + s*m;

   result =
      m;

  0.00 sec out of 0.00 sec
@tueda tueda added the bug Something isn't working label Jan 18, 2018
@jodavies
Copy link
Collaborator

The "works" ordering gives

==11536== Conditional jump or move depends on uninitialised value(s)
==11536==    at 0x4C1A48: GCDterms (ratio.c:2094)
==11536==    by 0x4C211E: GCDfunction3 (ratio.c:1179)
==11536==    by 0x4C3530: GCDfunction (ratio.c:1059)
==11536==    by 0x4B8F31: Generator (proces.c:3733)
==11536==    by 0x4B9473: Generator (proces.c:3931)
==11536==    by 0x4BA9F3: Processor (proces.c:404)
==11536==    by 0x438717: DoExecute (execute.c:838)
==11536==    by 0x44EF96: ExecModule (module.c:274)
==11536==    by 0x4B0A12: PreProcessor (pre.c:962)
==11536==    by 0x4EA969: main (startup.c:1605)
==11536== 
==11536== Conditional jump or move depends on uninitialised value(s)
==11536==    at 0x4C1A26: GCDterms (ratio.c:2095)
==11536==    by 0x4C211E: GCDfunction3 (ratio.c:1179)
==11536==    by 0x4C3530: GCDfunction (ratio.c:1059)
==11536==    by 0x4B8F31: Generator (proces.c:3733)
==11536==    by 0x4B9473: Generator (proces.c:3931)
==11536==    by 0x4BA9F3: Processor (proces.c:404)
==11536==    by 0x438717: DoExecute (execute.c:838)
==11536==    by 0x44EF96: ExecModule (module.c:274)
==11536==    by 0x4B0A12: PreProcessor (pre.c:962)
==11536==    by 0x4EA969: main (startup.c:1605)

whereas the "fails" ordering gives nothing, for me.

@benruijl
Copy link
Collaborator

Some more information: when using the "working order", tt2 is pointing to an entry in an uninitialized buffer, which causes the error. In the "working" order the value *tt2 is 0, and for the wrong order it is 22.

@jodavies
Copy link
Collaborator

jodavies commented Jan 24, 2018

I tested, but didn't find any cases, in which these issues (this and #260 ) mess up polyratfun. Can you confirm whether or not I should expect any changes in expressions making use of polyratfun?

@benruijl
Copy link
Collaborator

I'm afraid polyratfun is affected as well, at least for Bug #260. Bug #260 only happened when there was a univariate gcd in the first variable, which is rare.

This bug could occur when the numerator of the integer content is a small number, such that it matches the internal ID for symbols, vectors, etc. It is strange we didn't see this one before. You can test if this bug affects the polyratfun by putting a print in GCDterms. If it gets called during simplification of the polyratfun, you could be in trouble.

@tueda
Copy link
Collaborator

tueda commented Jan 24, 2018

I think GCDterms(), which was fixed for this issue, is not called from PolyRatFun, right?

@spj101
Copy link
Contributor Author

spj101 commented Jan 24, 2018

Thanks for the fast response @benruijl and @tueda.

On 8b945c1 this bug appears to be fixed. We also re-ran the original calculation that exposed this bug and we are now getting reasonable results.

tueda added a commit that referenced this issue Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants