#17695: Slightly fasten is_cyclotomic()
-------------------------------------+-------------------------------------
Reporter: bruno | Owner:
Type: enhancement | Status: needs_review
Priority: minor | Milestone: sage-6.5
Component: number theory | Resolution:
Keywords: cyclotomic | Merged in:
polynomials | Reviewers:
Authors: Bruno Grenet | Work issues:
Report Upstream: N/A | Commit:
Branch: | f79eeaaa630b1bd966a25a2f7b0f7a5f1dda3378
u/bruno/slightly_fasten_is_cyclotomic__| Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by vdelecroix):
Replying to [comment:11 bruno]:
> Replying to [comment:10 vdelecroix]:
> > Replying to [comment:9 bruno]:
> > > For concreteness, what do you propose to do now? If I understand
correctly, you propose to wait for #17631 and then make `is_cyclotomic()`
directly call the pari functions, and not introduce the tiny change I
made? I am fine with that, just wanted to be sure!
> >
> > For now, we can already do the thing I proposed with the gp interface
(and return the certificate instead of `True`/`False`). And we can also
add a `TODO` section in the documentation saying that we should move to
the pari interface as soon as #17631 is ready.
>
> I am not sure to agree since it actually makes the function slower:
> {{{#!python
> sage: p = cyclotomic_polynomial(150)
> sage: %timeit p.is_cyclotomic()
> 100 loops, best of 3: 2.75 ms per loop
> sage: %timeit p._gp_().poliscyclo()
> 100 loops, best of 3: 3.96 ms per loop
> }}}
Right. Most of the time seems to be lost in the conversion
{{{
sage: l = [cyclotomic_polynomial(i) for i in range(2,50)]
sage: timeit("for p in l: _ = p.is_cyclotomic()")
5 loops, best of 3: 51.7 ms per loop
sage: timeit("for p in l: _ = p._gp_().poliscyclo()")
5 loops, best of 3: 303 ms per loop
sage: lgp = [p._gp_() for p in l]
sage: timeit("for p in lgp: _ = p.poliscyclo()")
25 loops, best of 3: 34.8 ms per loop
}}}
> As an aside, isn't it a problem for a method named `is_cyclotomic()` to
return an integer rather than a boolean? Of course, the certificate is
interesting to have, and one can still test with `is_cyclotomic()` as
before. It is just that I expect a method `is_...()` to return a boolean.
Maybe I am wrong...
You are right. The best option would be:
{{{
def is_cyclotomic(self, certificate=False):
...
return ans if certificate else bool(ans)
}}}
A compromise can be:
- add the keyword `certificate` (if it is `True` we use
`self._gp_().polisyclco()`)
- do your initial suggested change
- add a `TODO` to the documentation to switch from gp to pari (and open a
ticket for it). We will then consider the fact of making it the default.
Vincent
--
Ticket URL: <http://trac.sagemath.org/ticket/17695#comment:12>
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.
