#5415: wrong definition of multifactorial?
------------------------------------+------------------------
Reporter: cwitty | Owner: robertwb
Type: defect | Status: new
Priority: major | Milestone: sage-6.4
Component: basic arithmetic | Resolution:
Keywords: | Merged in:
Authors: | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
Dependencies: | Stopgaps: todo
------------------------------------+------------------------
Comment (by nbruin):
Replying to [comment:14 prateek.cs14]:
> I have tried to redefine multifactorial function.
> Please review.
> https://github.com/sagemath/sage/pull/52
A few notes:
- Python is notoriously (and if you believe some of Guido's commentary
basicaly intentionally so) bad at recursion, so you should avoid it unless
your really need it. Here you don't (and the previous implementation
carefully avoids deep recursion)
- The current implementation does quite a bit of balancing of factors to
ensure good multiplication performance. From what I see on your github
branch, you've tacked on a new recursion case to what is called
"reflection" there, essentially making the main body (everything below it)
unreachable. That code looks like it's pretty carefully written. If you
want to throw it out you should do so (and not just leave it in there as
unreachable code) but then you should back up your proposal with some
serious timings that show your code performs better under all
circumstances.
- Whenever you make a change like this you should adjust the
documentation as well, ensuring that the new behaviour is reflected in the
documentation and is properly tested (probably adding some new tests that
differentiate between old and new behaviour.
I suspect that the appropriate change to make is in line 3967:
{{{
#!diff
- for i from 1 <= i <= n//k:
+ for i from 0 <= i <= n//k:
}}}
but I didn't check in detail.
--
Ticket URL: <http://trac.sagemath.org/ticket/5415#comment:15>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
