#9240: applying full_simplify() to gamma functions causes an error
---------------------------------------------------------------+------------
Reporter: tomc |
Owner: tomc
Type: defect |
Status: needs_review
Priority: major |
Milestone: sage-4.7.1
Component: symbolics |
Keywords: gamma function, full_simplify, factorial
Work_issues: |
Upstream: N/A
Reviewer: Dan Drake, Karl-Dieter Crisman, François Bissey |
Author: Tom Coates, Burcin Erocal
Merged: |
Dependencies: #11415
---------------------------------------------------------------+------------
Comment(by burcin):
Replying to [comment:12 kcrisman]:
> Thanks, I think that helps a ''little''. I also found
> {{{
> cdef _register_function(self):
> # We don't need to add anything to GiNaC's function registry
> # However, if any custom methods were provided in the python
class,
> # we should set the properties of the function_options object
> # corresponding to this function
> cdef GFunctionOpt opt =
g_registered_functions().index(self._serial)
>
> if hasattr(self, '_eval_'):
> opt.eval_func(self)
>
> }}}
> which I knew about before.
Francois is right. The `python_func` variable is a bitset, indexed by the
values here:
https://bitbucket.org/burcin/pynac/src/687b580c8c7c/ginac/function.h#cl-240
If the corresponding bit is on, then we call a python function.
When I first implemented this, defining custom evaluation etc. methods in
Python was an all or nothing matter. Then I realized that overriding some
of these methods and using the others defined in C++ made sense. I
switched to the bitset at that point. As the change with `&` and `&&`
shows, that switch wasn't very successful.
> I am going to have to write down '''exactly''' how all this works at
Sage Days 31, because I do not want to be rediscovering this from scratch
every time like I am now.
Maybe we can add some documentation to the reference manual about the
general design of symbolics and in particular how the functions work.
----
>
> I have some more questions, presumably for Burcin. I don't think they
are big deals, but I don't feel comfortable giving positive review without
knowing them. Someone else who knows more might!
>
> * why the change from the 'billions of digits' error message to the
symbolic answer? This seems like a big change - someone might rely on
that type of entry failing in number theory. Note that the multifactorial
still has the 'billions of digits' error message, incidentally.
The number theory people should use the functions from `sage.rings.arith`.
In general, it's a bad idea to use the symbolics functions in library code
unless you know what you're doing.
Suppose you have the expression
`factorial(big_number+1)/factorial(big_number)`. This would simplify to
`big_number`. Telling people that they can't possibly work with numbers
that big defeats the purpose of ''symbolic computation''.
> * what would the problem be if Ginac got symbolic answers back, if it
didn't have anything for those before? (Not criticizing, just not
understanding. I don't have a problem with them being numeric for ints
and floats and longs.)
We get problems like #9913. I was being cautious.
> * Why did you remove `opt.set_python_func() `? I assume this has
something to do with fbissey's comment.
> * Does ` return None ` just mean that Ginac will not try to
evaluate things like `factorial(sqrt(2))` internally?
Yes. It's quite likely that this is not documented anywhere. :)
----
>
> Status:
> * Positive review on Tom's patch, from Dan Drake.
> * The log gamma stuff is fine.
> * Apparently Francois is happy with the && to & switch. This is beyond
me, though I don't see any problems with it.
> * The actual changes to and new factorial and gamma functions are fine.
> * Need answer to questions, or someone else to review those pieces in
lieu of that.
> * Finally, the big question - WHY this change? I can't find a single
doctest that tells me what broke with Tom's patch but without Burcin's
patch. I feel there must be some very subtle Maxima output that could
have come out incorrect, but I cannot find it. All these doctests should
have worked before (or were cdef functions so they couldn't be doctested).
Tom's patch was great, it fixed the problem at hand. But when I first
looked at it, I thought it needed more tests. Sitting down to write the
tests, I found all kinds of problems, like the bitwise `&` issue and the
big factorials raising an error. So I fixed those as well. Perhaps it was
a mistake to tag these changes on this ticket, but here we are now.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/9240#comment:13>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
