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: sage-6.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 burcin)

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 sage-support 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*x-1/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/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.__call__ (sage/symbolic/expression.cpp:15657)()

/Applications/sage/local/lib/python2.6/site-packages/sage/symbolic/ring.so in sage.symbolic.ring.SymbolicRing._call_element_ (sage/symbolic/ring.cpp:6537)()

/Applications/sage/local/lib/python2.6/site-packages/sage/symbolic/expression.so in sage.symbolic.expression.Expression.substitute (sage/symbolic/expression.cpp:15036)()

/Applications/sage/local/lib/python2.6/site-packages/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(-x-1/2)*g
gamma(-x - 1/2)^2
sage: g(x=0)
-2*sqrt(pi)

That is, merely performing the should-be-irrelevant "gamma(-x-1/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.

Apply trac_10849-doctests-on_9880.patch

Attachments (1)

trac_10849-doctests-on_9880.patch (845 bytes) - added by burcin 7 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by mhansen

I'm pretty sure this issue stems from comparison of number field elements. See #6132, #7160, #10064, and http://groups.google.com/group/sage-support/browse_thread/thread/28bbd04a78dadb57/01168722573ff736

comment:2 Changed 9 years ago by dsm

What weirds me out is the side-effect aspect: I could understand getting the same wrong answer, or different answers for equivalent but non-identical inputs, or even random behaviour. I don't see why the gamma(-x-1/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 burcin

  • Milestone set to sage-4.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 kcrisman

  • Cc kcrisman added

comment:5 Changed 8 years ago by burcin

  • Authors set to Burcin Erocal
  • Dependencies set to #7160
  • Description modified (diff)
  • Milestone changed from sage-4.8 to sage-5.0
  • Status changed from new to needs_review

This is fixed with the patch attached to #7160. Doctests are in attachment:trac_10849-doctests.patch.

comment:6 Changed 7 years ago by tkluck

  • 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 kcrisman

One can also give it positive review and set it to sage-pending like at #10064. That's a little more informative, anyway.

comment:8 follow-up: Changed 7 years ago by 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.

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 kcrisman

  • Milestone changed from sage-5.6 to sage-pending

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 sage-pending, and you set it to positive review if you feel that way? :-)

Changed 7 years ago by burcin

comment:10 Changed 7 years ago by burcin

  • Dependencies changed from #7160 to #7160, #9880
  • Description modified (diff)
  • Milestone changed from sage-pending to sage-5.11
  • Status changed from needs_work to needs_review

Now that #7160 is fixed, this can be reviewed. Patchbot says that the old patch applies without problems on Sage 5.10.beta4 and passes tests. I am attaching a new patch that is rebased on #9880, since both tickets add doctests to the same function and I hope #9880 will go in before this one. :)

comment:11 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:13 Changed 6 years ago by rws

  • 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 rws

  • 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:

dfaa889trac 10849: add doctests for fix to number field element comparison

comment:15 Changed 6 years ago by git

  • 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:

79a0ebdMerge branch 'develop' into ticket/10849

comment:16 Changed 6 years ago by rws

  • Status changed from needs_review to positive_review

comment:17 Changed 6 years ago by vbraun

  • Branch changed from u/rws/ticket/10849 to 79a0ebda9d793ed3730584d522d8a5ba0c71ee26
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.