#20371: dispersion and dispersion sets of polynomials
-------------------------------------+-------------------------------------
Reporter: mmezzarobba | Owner:
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-7.2
Component: algebra | Resolution:
Keywords: | Merged in:
Authors: Marc Mezzarobba | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/mmezzarobba/20371-dispersion | 12dbbd7e433743d27324eb91bad13647abc5f255
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by mmezzarobba):
Hi Bruno,
Replying to [comment:2 bruno]:
> A few remarks/questions:
>
> * In the docstrings of `dispersion_set` and `dispersion` you have an
inconsistency between `p,q` and `f,g`:
> {{{
> The dispersion set of `p` and `q` is the set of nonnegative integers
> `n` such that `f(x + n)` and `g(x)` have a nonconstant common factor.
> }}}
Thanks, I will fix that.
> * It may be useful to mention that when the dispersion set is empty, the
dispersion you return is `-Infinity`. There is an example of the behavior,
but you may add it in the text.
Since there is nothing really surprising about that return value, I'd
prefer keeping just the example. But please feel free to insist if you
disagree.
> * You use `assert` twice: Is there an example where the assertion does
not hold? If this is the case, it may be interesting to have a more
meaningful error message. If this is just to prevent an hypothetic future
case where some factorization algorithm does not respect this assertion,
is it really needed?
I don't know if there is such an example. Since this is a generic
implementation, I'd rather be on the safe side. And if you are concerned
with the performance penalty, you can always run python with assertions
disabled...
> * In the first line of dispersion_set, you use a possibly useless
coercion. You may change this line to (I was not sure about the syntax but
it seems to work):
> {{{#!python
> other = self if other is None else other if other in self._parent else
self._parent.coerce(other)
> }}}
As far as I understand, your version is not equivalent to mine (`QQ(1) in
ZZ` holds), and actually does more work: `in` alone will perform a
conversion and a coercion, or something like that.
--
Ticket URL: <http://trac.sagemath.org/ticket/20371#comment:3>
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 https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
