Opened 9 years ago
Closed 6 years ago
#10849 closed defect (fixed)
behaviour of gamma strangely sensitive
Reported by:  dsm  Owned by:  burcin 

Priority:  major  Milestone:  sage6.2 
Component:  symbolics  Keywords:  doctest 
Cc:  kcrisman  Merged in:  
Authors:  Burcin Erocal  Reviewers:  Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  79a0ebd (Commits)  Commit:  79a0ebda9d793ed3730584d522d8a5ba0c71ee26 
Dependencies:  #7160, #9880  Stopgaps: 
Description (last modified by )
Whether you get the correct result or a spurious ValueError? in evaluating some gamma expressions depends upon things it shouldn't.
This comes from a sagesupport thread; I looked and couldn't find it here, but possibly it's a duplicate. (I have bad luck with trac.) See also this worksheet.
Here's a relatively simple test case, in 4.6.1 and 4.6.2.rc0:
sage: x = var("x") sage: f = exp(gamma(I*x1/2).subs(x=I*x)) sage: g=f.operands()[0] sage: g, type(g) (gamma(x  1/2), <type 'sage.symbolic.expression.Expression'>) sage: g(x=0)  ValueError Traceback (most recent call last) /Applications/sage_devel/<ipython console> in <module>() /Applications/sage/local/lib/python2.6/sitepackages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__call__ (sage/symbolic/expression.cpp:15657)() /Applications/sage/local/lib/python2.6/sitepackages/sage/symbolic/ring.so in sage.symbolic.ring.SymbolicRing._call_element_ (sage/symbolic/ring.cpp:6537)() /Applications/sage/local/lib/python2.6/sitepackages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.substitute (sage/symbolic/expression.cpp:15036)() /Applications/sage/local/lib/python2.6/sitepackages/sage/symbolic/pynac.so in sage.symbolic.pynac.py_doublefactorial (sage/symbolic/pynac.cpp:9463)() ValueError: argument must be >= 1 sage: g(x=0)  ValueError Traceback (most recent call last) [ ... duplicate removed; this is merely to show it's not only the first call which has problems ] ValueError: argument must be >= 1 sage: gamma(x1/2)*g gamma(x  1/2)^2 sage: g(x=0) 2*sqrt(pi)
That is, merely performing the shouldbeirrelevant "gamma(x1/2)*g" op makes g(x=0) start working. Instrumenting it shows that that py_doublefactorial gets a 3 the first time instead of the 1 it does the second time, but I couldn't quite follow the calling order to isolate the error.
Attachments (1)
Change History (18)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
What weirds me out is the sideeffect aspect: I could understand getting the same wrong answer, or different answers for equivalent but nonidentical inputs, or even random behaviour. I don't see why the gamma(x1/2)*g call should change the state so that any element comparison would behave differently, but I understand exactly nothing about Sage internals at this level.
comment:3 Changed 9 years ago by
 Milestone set to sage4.7.1
Mike is right and the patch at #10064 resolves this issue.
The side effect is a result of GiNaC's reference counted pointers. Whenever two expressions compare equal, GiNaC frees the memory of one, and makes it a pointer to the other. In this example, it replaces x  1/2
where the coefficient of x
is in QQ(i), with x  1/2
where the coefficient is just an int
.
sage: t = x 1/2 sage: t x  1/2 sage: t.operands() [x, 1/2] sage: t.operands()[0] x sage: t.operands()[0].operands() [x, 1] sage: t.operands()[0].operands()[1] 1 sage: t.operands()[0].operands()[1].pyobject() 1 sage: type(t.operands()[0].operands()[1].pyobject()) <type 'int'>
Of course there is a chance that this observation changed the result. :)
For g
, this looks like a different bug:
sage: g gamma(x  1/2) sage: g.operands()[0] x  1/2 sage: g.operands()[0].operands() [x, 1/2] sage: g.operands()[0].operands()[0] x sage: g.operands()[0].operands()[0].operands() []
comment:4 Changed 9 years ago by
 Cc kcrisman added
comment:5 Changed 8 years ago by
 Dependencies set to #7160
 Description modified (diff)
 Milestone changed from sage4.8 to sage5.0
 Status changed from new to needs_review
This is fixed with the patch attached to #7160. Doctests are in attachment:trac_10849doctests.patch.
comment:6 Changed 7 years ago by
 Status changed from needs_review to needs_work
I'll set this one to needs work because it depends on a ticket (#7160) that needs work, and it can't be reviewed before that one.
comment:7 Changed 7 years ago by
One can also give it positive review and set it to sagepending like at #10064. That's a little more informative, anyway.
comment:8 followup: ↓ 9 Changed 7 years ago by
We'll still have to check whether the patch still applies cleanly when the other ticket has been merged (at which point a lot of unrelated changes may have made their way into Sage too). That would keep me from giving it the sort of preliminary positive review you suggest.
But if what you're saying is the standard way of handling these things, then I'll raise no objection.
comment:9 in reply to: ↑ 8 Changed 7 years ago by
 Milestone changed from sage5.6 to sagepending
Replying to tkluck:
We'll still have to check whether the patch still applies cleanly when the other ticket has been merged (at which point a lot of unrelated changes may have made their way into Sage too). That would keep me from giving it the sort of preliminary positive review you suggest.
Of course you're right! But at least this would mean that modulo those other changes, the patch does what it says. Otherwise we might lose the info of a positive review.
How about I set it to sagepending, and you set it to positive review if you feel that way? :)
Changed 7 years ago by
comment:10 Changed 7 years ago by
 Dependencies changed from #7160 to #7160, #9880
 Description modified (diff)
 Milestone changed from sagepending to sage5.11
 Status changed from needs_work to needs_review
comment:11 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:12 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:13 Changed 6 years ago by
 Branch set to u/rws/ticket/10849
 Modified changed from 01/30/14 21:20:52 to 01/30/14 21:20:52
comment:14 Changed 6 years ago by
 Commit set to dfaa88920b4be4ff22103008c086fb1837144400
 Keywords doctest added
 Reviewers set to Ralf Stephan
 Status changed from needs_review to positive_review
This is just a doctest which passes.
New commits:
dfaa889  trac 10849: add doctests for fix to number field element comparison

comment:15 Changed 6 years ago by
 Commit changed from dfaa88920b4be4ff22103008c086fb1837144400 to 79a0ebda9d793ed3730584d522d8a5ba0c71ee26
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
79a0ebd  Merge branch 'develop' into ticket/10849

comment:16 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:17 Changed 6 years ago by
 Branch changed from u/rws/ticket/10849 to 79a0ebda9d793ed3730584d522d8a5ba0c71ee26
 Resolution set to fixed
 Status changed from positive_review to closed
I'm pretty sure this issue stems from comparison of number field elements. See #6132, #7160, #10064, and http://groups.google.com/group/sagesupport/browse_thread/thread/28bbd04a78dadb57/01168722573ff736