#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 | bfff2a33749d9637994735eff5449851bf0c8155
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by bruno):
Hi Marc,
Replying to [comment:3 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.
You only fixed the first typo...
>
> > * 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.
I do not have strong opinion on this...
> > * 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...
It was more a matter of clarity. Again, I do not have a strong opinion...
>
> > * 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.
Right, I did not write it correctly. What I meant:
{{{#!python
other = self if other is None else other if other._parent is self._parent
else self._parent.coerce(other)
}}}
That way, I guess that my version is completely equivalent to yours and
(slightly) faster. I may be stuttering but I still don't have a strong
opinion on whether you should change your code...
--
Ticket URL: <http://trac.sagemath.org/ticket/20371#comment:5>
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.
