Sage: Ticket #10771: gcd and lcm for fraction fields
https://trac.sagemath.org/ticket/10771
<p>
At <a class="ext-link" href="http://groups.google.com/group/sage-devel/browse_thread/thread/cd05585cf395b3a0/a34f04f32d68e525"><span class="icon"></span>sage-devel</a>, the question was raised whether it really is a good idea that the gcd in the rational field should return either <code>0</code> or <code>1</code>.
</p>
<p>
Since <em>any</em> non-zero element of <code>QQ</code> qualifies as gcd of two non-zero rationals, it should be possible to define gcd and lcm, so that <code>gcd(x,y)*lcm(x,y)==x*y</code> holds for any rational numbers x,y, and so that <code>gcd(QQ(m),QQ(n))==gcd(m,n)</code> and <code>lcm(QQ(m),QQ(n))==lcm(m,n)</code> for any two integers m,n.
</p>
<p>
Moreover, it should be possible to provide gcd/lcm for any fraction field of a <code>PID</code>: Note that currently gcd raises a type error for elements of <code>Frac(QQ['x'])</code>.
</p>
<p>
The aim is to implement gcd and lcm as <code>ElementMethods</code> of the category <code>QuotientFields()</code>.
</p>
<p>
<strong><span class="underline">Approach</span></strong>
</p>
<p>
Let R be an integral domain, assume that it provides gcd and lcm, and let F be its fraction field. Since R has gcd, we can assume that <code>x.numerator()</code> and <code>x.denominator()</code> are relatively prime for any element x of F.
</p>
<p>
Then, define
</p>
<pre class="wiki">gcd(x,y) = gcd(x.numerator(),y.numerator())/lcm(x.denominator(),y.denominator())
lcm(x,y) = lcm(x.numerator(),y.numerator())/gcd(x.denominator(),y.denominator())
</pre><p>
<strong><span class="underline">Benefits</span></strong>
</p>
<p>
If that approach is mathematically sober, we obtain the following equalities up to units in R:
</p>
<ul><li><code>gcd(x,y)*lcm(x,y)==x*y</code>, for any x,y in F, provided that the equality holds for any x,y in R.
</li><li><code>gcd(F(x),F(y))==gcd(x,y)</code> and <code>lcm(F(x),F(y))==lcm(x,y)</code> for any x,y in R.
</li></ul>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10771
Trac 1.1.6SimonKingFri, 11 Feb 2011 10:22:56 GMTdescription changed
https://trac.sagemath.org/ticket/10771#comment:1
https://trac.sagemath.org/ticket/10771#comment:1
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10771?action=diff&version=1">diff</a>)
</li>
</ul>
TicketSimonKingMon, 14 Feb 2011 13:12:19 GMTstatus, type changed; author set
https://trac.sagemath.org/ticket/10771#comment:2
https://trac.sagemath.org/ticket/10771#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>type</strong>
changed from <em>PLEASE CHANGE</em> to <em>defect</em>
</li>
<li><strong>author</strong>
set to <em>Simon King</em>
</li>
</ul>
<p>
Without the patch, we had:
</p>
<pre class="wiki">sage: gcd(2/1,4) # does not restrict to ZZ
1
sage: lcm(2/1,4) # Bug
Traceback (most recent call last):
...
TypeError: Argument 'other' has incorrect type (expected sage.rings.rational.Rational, got sage.rings.integer.Integer)
sage: R.<x> = QQ[]
sage: lcm(1/(x+1),1/(x+1)^2) # note that the error message names gcd, not lcm!
Traceback (most recent call last):
...
TypeError: unable to find gcd of 1/(x + 1) and 1/(x^2 + 2*x + 1)
sage: gcd(1/(x+1),1/(x+1)^2)
Traceback (most recent call last):
...
TypeError: unable to find gcd of 1/(x + 1) and 1/(x^2 + 2*x + 1)
sage: gcd(int(2),2/1)
2
sage: gcd(2,2/1) # gcd of ints and integers are different
1
</pre><p>
With the patch, one has
</p>
<pre class="wiki">sage: gcd(2/1,4)
2
sage: lcm(2/1,4)
4
sage: R.<x> = QQ[]
sage: lcm(1/(x+1),1/(x+1)^2)
1/(x + 1)
sage: gcd(1/(x+1),1/(x+1)^2)
1/(x^2 + 2*x + 1)
sage: gcd(int(2),2/1)
2
sage: gcd(2,2/1)
2
</pre>
TicketmstrengMon, 14 Feb 2011 14:48:31 GMTreviewer set
https://trac.sagemath.org/ticket/10771#comment:3
https://trac.sagemath.org/ticket/10771#comment:3
<ul>
<li><strong>reviewer</strong>
set to <em>Marco Streng</em>
</li>
</ul>
<p>
A couple of comments:
</p>
<ul><li>I don't see the point of keeping a special function <code>gcd_rational(self, other, **kwds):</code> that returns a GCD in the set {0,1}. Why only for QQ? Why at all? Also, the difference between <code>gcd</code> and <code>gcd_rational</code> is not explained by the word "rational". Perhaps <code>gcd_zero_one</code> would be a more informative name.
</li></ul><ul><li>Everything generalizes from principal ideal domains to unique factorization domains (UFD's) (such as multivariate polynomial rings over unique factorization domains) as long as they have <code>gcd</code> and <code>lcm</code> methods implemented. Why not write "unique factorization domain" in the documentation instead of "principal ideal domain"?
</li></ul><ul><li>Suppose I have a fraction field F of a ring of type R that does not have lcm and gcd methods, or these methods exist, but raise other kinds of errors, e.g. because the ring is not a UFD or the methods have not been implemented. Let a and b be elements of F. Then a and b have a gcd in F because F is a field, so I would expect a.gcd(b) to return something (anything basically). After applying your patch, if I do <code>a.gcd(b)</code>, it is very confusing to get an <code>AttributeError: 'RElement' object has no attribute 'gcd'</code>: I'm not interested in gcd's of RElements, only of <code>(Fraction)FieldElements</code>. You could put your entire <code>gcd</code> and <code>lcm</code> code between <code>try:</code> and <code>except (AttributeError, NotImplementedError, TypeError, ValueError):</code> to return the same 0 or 1 that <code>gcd_rational</code> would (which is a mathematically correct gcd in F after all).
</li></ul><ul><li>trac has a <code>t</code> too much in line 1610 of ring.pyx: <code>the case of the rational field. However, since tract ticket #10771,</code>
</li></ul><ul><li>you write "quotient field" in the documentation. You could write "fraction field" to avoid confusion with quotient rings, which may be fields. This makes it more clear that you refer to the mathematical counterpart of Sage's "<code>FractionField</code>"?
</li></ul><ul><li><code>All tests passed!</code> with -long, well documented, looks good.
</li></ul>
TicketlftaberaMon, 14 Feb 2011 14:55:38 GMT
https://trac.sagemath.org/ticket/10771#comment:4
https://trac.sagemath.org/ticket/10771#comment:4
<p>
Related tickets: <a class="closed ticket" href="https://trac.sagemath.org/ticket/9819" title="enhancement: Add a default gcd and lcm methods for fields (closed: duplicate)">#9819</a>, <a class="closed ticket" href="https://trac.sagemath.org/ticket/10459" title="defect: serious troubles with gcd (closed: duplicate)">#10459</a>
</p>
TicketdsmMon, 14 Feb 2011 15:43:54 GMT
https://trac.sagemath.org/ticket/10771#comment:5
https://trac.sagemath.org/ticket/10771#comment:5
<p>
Trivial: typo "tract" for "trac" in patch.
</p>
TicketSimonKingMon, 14 Feb 2011 15:49:38 GMT
https://trac.sagemath.org/ticket/10771#comment:6
https://trac.sagemath.org/ticket/10771#comment:6
<p>
Hi Marco,
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:3" title="Comment 3">mstreng</a>:
</p>
<blockquote class="citation">
<p>
A couple of comments:
</p>
<ul><li>I don't see the point of keeping a special function <code>gcd_rational(self, other, **kwds):</code> that returns a GCD in the set {0,1}. Why only for QQ? Why at all? Also, the difference between <code>gcd</code> and <code>gcd_rational</code> is not explained by the word "rational". Perhaps <code>gcd_zero_one</code> would be a more informative name.
</li></ul></blockquote>
<p>
Perhaps I am too 'conservative': I would rather rename something than to delete it.
</p>
<p>
But I suppose it would be better to implement a gcd and an lcm as element methods of <code>Fields()</code> -- see <a class="closed ticket" href="https://trac.sagemath.org/ticket/9819" title="enhancement: Add a default gcd and lcm methods for fields (closed: duplicate)">#9819</a>. In that way, one could easily provide a correct gcd/lcm behaviour for all fields, and for fraction fields of PID one would still obtain a gcd/lcm behaviour that is not only correct but nice.
</p>
<blockquote class="citation">
<ul><li>Everything generalizes from principal ideal domains to unique factorization domains (UFD's) (such as multivariate polynomial rings over unique factorization domains) as long as they have <code>gcd</code> and <code>lcm</code> methods implemented. Why not write "unique factorization domain" in the documentation instead of "principal ideal domain"?
</li></ul></blockquote>
<p>
Good question. I did it by accident. However, it matches the sad fact that the categories are a little askew here:
</p>
<pre class="wiki">sage: PrincipalIdealDomains().is_subcategory(UniqueFactorizationDomains())
False # shouldn't that be "True"?
sage: PrincipalIdealDomains().is_subcategory(GcdDomains())
True
sage: UniqueFactorizationDomains().is_subcategory(GcdDomains())
True
</pre><p>
Perhaps one should write "fraction field of an integral domain with gcd and lcm"? Because that's what is duck typed.
</p>
<blockquote class="citation">
<ul><li>Suppose I have a fraction field F of a ring of type R that does not have lcm and gcd methods, or these methods exist, but raise other kinds of errors, e.g. because the ring is not a UFD or the methods have not been implemented. Let a and b be elements of F. Then a and b have a gcd in F because F is a field, so I would expect a.gcd(b) to return something (anything basically). After applying your patch, if I do <code>a.gcd(b)</code>, it is very confusing to get an <code>AttributeError: 'RElement' object has no attribute 'gcd'</code>: I'm not interested in gcd's of RElements, only of <code>(Fraction)FieldElements</code>. You could put your entire <code>gcd</code> and <code>lcm</code> code between <code>try:</code> and <code>except (AttributeError, NotImplementedError, TypeError, ValueError):</code> to return the same 0 or 1 that <code>gcd_rational</code> would (which is a mathematically correct gcd in F after all).
</li></ul></blockquote>
<p>
If one adds a gcd and lcm for field elements (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9819" title="enhancement: Add a default gcd and lcm methods for fields (closed: duplicate)">#9819</a>) returning 0 or 1, then your suggestion certainly makes sense.
</p>
<blockquote class="citation">
<ul><li>trac has a <code>t</code> too much in line 1610 of ring.pyx: <code>the case of the rational field. However, since tract ticket #10771,</code>
</li></ul><ul><li>you write "quotient field" in the documentation. You could write "fraction field" to avoid confusion with quotient rings, which may be fields. This makes it more clear that you refer to the mathematical counterpart of Sage's "<code>FractionField</code>"?
</li></ul></blockquote>
<p>
Thanks! I am about to post a new patch, and I hope that Luis as author of <a class="closed ticket" href="https://trac.sagemath.org/ticket/9819" title="enhancement: Add a default gcd and lcm methods for fields (closed: duplicate)">#9819</a> does not mind if I add the case of arbitrary fields to my patch.
</p>
TicketSimonKingMon, 14 Feb 2011 16:19:22 GMT
https://trac.sagemath.org/ticket/10771#comment:7
https://trac.sagemath.org/ticket/10771#comment:7
<p>
I just updated my patch, and I hope it addresses all your remarks. The typos are fixed, the old gcd of the rational field is now removed, and: I also added gcd/lcm for general fields. I am sorry that this makes <a class="closed ticket" href="https://trac.sagemath.org/ticket/9819" title="enhancement: Add a default gcd and lcm methods for fields (closed: duplicate)">#9819</a> a duplicate.
</p>
<p>
And I hope that I did not confuse gcd and lcm in the following setting:
</p>
<pre class="wiki">sage: GF(2)(1).gcd(GF(2)(1))
1
sage: GF(2)(1).gcd(GF(2)(0))
1
sage: GF(2)(0).gcd(GF(2)(0))
0
sage: GF(2)(1).lcm(GF(2)(0))
0
sage: GF(2)(1).lcm(GF(2)(1))
1
</pre><p>
That's implemented as element methods for the category of <code>Fields()</code>. Somehow, the category framework is cool, isn't it?
</p>
TicketSimonKingMon, 14 Feb 2011 16:23:33 GMT
https://trac.sagemath.org/ticket/10771#comment:8
https://trac.sagemath.org/ticket/10771#comment:8
<p>
Oops, I forgot one aspect: If the gcd/lcm of the base ring raises an error, it should be caught. Implementing it now...
</p>
TicketSimonKingMon, 14 Feb 2011 16:34:16 GMT
https://trac.sagemath.org/ticket/10771#comment:9
https://trac.sagemath.org/ticket/10771#comment:9
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:8" title="Comment 8">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Oops, I forgot one aspect: If the gcd/lcm of the base ring raises an error, it should be caught. Implementing it now...
</p>
</blockquote>
<p>
Here it is:
</p>
<pre class="wiki">sage: R.<q> = ZZ.extension(x^2+5)
sage: gcd(q,q)
1
sage: gcd(q,0)
1
sage: gcd(R.zero(),0)
0
sage: lcm(q,q)
1
sage: lcm(q,0)
0
</pre><p>
Correct me if I am wrong, but I think that now every complaint is addressed.
</p>
<p>
Best regards,
</p>
<p>
Simon
</p>
TicketmstrengMon, 14 Feb 2011 17:07:19 GMTstatus changed
https://trac.sagemath.org/ticket/10771#comment:10
https://trac.sagemath.org/ticket/10771#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
New complaints, sorry.
</p>
<ul><li>I don't really like this:
<pre class="wiki">sage: M = Matrix(GF(3),2,2,[1,2,0,1]); M
[1 2]
[0 1]
sage: GF(2)(1).lcm(M)
1
</pre>I think you should first try to coerce other to self.parent() in gcd and lcm for Field elements.
</li></ul><ul><li>Line 64 of quotient_fields.py "<code>both GCD and LCM, it is possible to be a bit more specific</code>"
uses capital letters GCD and LCM, while the methods that it (quasi-)refers to are lower case:
<code>AttributeError: 'sage.rings.rational.Rational' object has no attribute 'GCD'</code>
</li></ul><ul><li>Next line: "define the GCD uniquely up to a unit in the base ring". Write "of" instead of "in"
so that it is clear that you are not saying that the GCD is in the base ring.
</li></ul><ul><li>Your tests with <code>R.<q> = ZZ.extension(x^2+5)</code> is unrelated to fraction field: q is 1
<pre class="wiki">sage: R.<q> = ZZ.extension(x^2+5)
sage: g = R.gens(); g
[1, q]
sage: q
1
sage: g[1]
q
sage: g[1] == q
False
</pre>All your examples involving q get coerced to ZZ by the global GCD and LCM, so you should
find other examples. This seems to work:
<pre class="wiki">sage: gcd(g[1],1)
TypeError: unable to find gcd of q and 1
sage: gcd(g[1]/1,1)
1
</pre>(Correct behaviour both times)
</li></ul><p>
Time to catch a bus, may have more comments later.
</p>
TicketlftaberaMon, 14 Feb 2011 17:22:26 GMT
https://trac.sagemath.org/ticket/10771#comment:11
https://trac.sagemath.org/ticket/10771#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:7" title="Comment 7">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
I just updated my patch, and I hope it addresses all your remarks. The typos are fixed, the old gcd of the rational field is now removed, and: I also added gcd/lcm for general fields. I am sorry that this makes <a class="closed ticket" href="https://trac.sagemath.org/ticket/9819" title="enhancement: Add a default gcd and lcm methods for fields (closed: duplicate)">#9819</a> a duplicate.
</p>
</blockquote>
<p>
Please.
</p>
<blockquote class="citation">
<p>
And I hope that I did not confuse gcd and lcm in the following setting:
</p>
<pre class="wiki">sage: GF(2)(1).gcd(GF(2)(1))
1
sage: GF(2)(1).gcd(GF(2)(0))
1
sage: GF(2)(0).gcd(GF(2)(0))
0
sage: GF(2)(1).lcm(GF(2)(0))
0
sage: GF(2)(1).lcm(GF(2)(1))
1
</pre><p>
That's implemented as element methods for the category of <code>Fields()</code>. Somehow, the category framework is cool, isn't it?
</p>
</blockquote>
<p>
I do not full understand this, but I am happy that works.
</p>
<p>
Some thoughts:
</p>
<p>
For QQ and the like, could it be that gcd and lcm should only take care of coercing to a good setting and then the real algorithm should be in _lcm, _gcd? Note that QQ already has ._gcd and ._lcm so these methods has to be taken into account.
</p>
<p>
For generic Fields, It should appear in the documentation that the methods return 0 if both arguments are zero and a non-zero element otherwise. The user should not suppose that the non-zero element is actually one, since this is changed by subclasses.
</p>
TicketSimonKingTue, 15 Feb 2011 10:22:50 GMT
https://trac.sagemath.org/ticket/10771#comment:12
https://trac.sagemath.org/ticket/10771#comment:12
<p>
Hi Luis!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:11" title="Comment 11">lftabera</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
That's implemented as element methods for the category of <code>Fields()</code>. Somehow, the category framework is cool, isn't it?
</p>
</blockquote>
<p>
I do not full understand this, but I am happy that works.
</p>
</blockquote>
<p>
Then let me explain:
</p>
<p>
Any category (in Sage) comes with a parent class and an "element class". That very often is an abstract thing, but you can inherit from it, and in particular you can implement methods for it.
</p>
<p>
If things are properly done, then a parent P is declared as an object in some category, and its class <em>automatically(!)</em> inherits from the parent class of its category:
</p>
<pre class="wiki">sage: QQ.__class__
<class 'sage.rings.rational_field.RationalField_with_category'>
sage: issubclass(QQ.__class__,QuotientFields().parent_class)
True
</pre><p>
If things are very properly done, then the parent has an attribute <code>Element</code>, which is supposed to be a class. That class will (again automatically) be translated into a subclass of the element class of the parent's category, and is available as the attribute <code>element_class</code>:
</p>
<pre class="wiki">sage: m = matrix(GF(2),[[1]])
sage: G = MatrixGroup([m])
sage: G.Element
<class 'sage.groups.matrix_gps.matrix_group_element.MatrixGroupElement'>
sage: G.__class__
<class 'sage.groups.matrix_gps.matrix_group.MatrixGroup_gens_finite_field_with_category'>
sage: G.element_class
<class 'sage.groups.matrix_gps.matrix_group_element.MatrixGroup_gens_finite_field_with_category.element_class'>
sage: issubclass(G.element_class,G.Element)
True
sage: issubclass(G.element_class,G.category().element_class)
True
sage: isinstance(G.random_element(),G.category().element_class)
True
</pre><p>
So, if things are very properly done, then a method defined as an "element method" of a category, will be available by looking up the method resolution order, since the elements of the parent are actual instances of the category's element class.
</p>
<p>
Sometimes things are done properly, but not <em>very</em> properly:
</p>
<pre class="wiki">sage: QQ.category()
Category of quotient fields
sage: isinstance(QQ, QQ.category().parent_class)
True
sage: hasattr(QQ,'Element')
False
sage: isinstance(QQ.one(), QQ.category().element_class)
False
</pre><p>
Hence, the methods that I defined for the element class of the "category of quotient fields" is not available to elements of <code>QQ</code> by simply looking up the method resolution order. But: There also is a <code>__getattr__</code> method implemented for <code>sage.structure.element.Element</code>, and this can access the element methods of the category of the parent of an element (uff!) even if the element is no instance of the "proper" element class.
</p>
<p>
This is why the new gcd works for the rationals, but it's slower than with the method resolution order, and this is why I also did not remove the existing lcm for rationals.
</p>
<blockquote class="citation">
<p>
Some thoughts:
</p>
<p>
For QQ and the like, could it be that gcd and lcm should only take care of coercing to a good setting and then the real algorithm should be in _lcm, _gcd? Note that QQ already has ._gcd and ._lcm so these methods has to be taken into account.
</p>
</blockquote>
<p>
That's a nice observation. By searching a little, I found that <code>_gcd</code> is used in the method <code>sage.structure.element.PrincipalIdealDomainElement.gcd</code>.
</p>
<p>
However, there are only very few classes that define a <code>_gcd</code> method: <code>sage.structure.element.EuclideanDomainElement</code>, <code>sage.structure.element.FieldElement</code>, <code>sage.rings.rational.Rational</code>, and then there are hits in <code>...polynomial_element_generic</code> and <code>...polynomial_modn_dense_ntl</code>.
</p>
<p>
In other words, I guess that the <code>_gcd</code> method is an artefact of earlier attempts to reflect mathematics in the class hierarchy -- this should now be done in the category framework. <code>_gcd</code> will <em>only</em> play a role if
</p>
<ol><li>the class of an element derives from <code>PrincipalIdealDomainElement</code>, and
</li></ol><ol start="2"><li>the class of an element additionally defines <code>_gcd</code>.
</li></ol><p>
It seems to me that there is precisely one class that directly inherits from <code>PrincipalIdealDomainElement</code>, and there is precisely one class that directly inherits from that class:
</p>
<pre class="wiki">sage: search_src("\(PrincipalIdealDomainElement")
structure/element.pxd:83:cdef class EuclideanDomainElement(PrincipalIdealDomainElement):
structure/element.pyx:2370:cdef class EuclideanDomainElement(PrincipalIdealDomainElement):
sage: search_src("\(EuclideanDomainElement")
structure/element.pyx:2362:PY_SET_TP_NEW(EuclideanDomainElement, Element)
rings/integer.pxd:9:cdef class Integer(EuclideanDomainElement):
</pre><p>
So, <code>_gcd</code> is only used for the <code>Integer</code> class. I guess it could easily be removed.
</p>
<blockquote class="citation">
<p>
For generic Fields, It should appear in the documentation that the methods return 0 if both arguments are zero and a non-zero element otherwise. The user should not suppose that the non-zero element is actually one, since this is changed by subclasses.
</p>
</blockquote>
<p>
Right. I'll take care of that, but now I have a bus to catch...
</p>
TicketmstrengTue, 15 Feb 2011 12:09:22 GMT
https://trac.sagemath.org/ticket/10771#comment:13
https://trac.sagemath.org/ticket/10771#comment:13
<ul><li>Did you run a full doctest? I'm getting doctest failures that may have something to do with your patch, as they probably simplify fractions somewhere deep inside in some way. Or my installation is messed up again.
<pre class="wiki">sage -t -long devel/sage/sage/symbolic/expression.pyx # 1 doctests failed
sage -t -long devel/sage/sage/symbolic/integration/integral.py # 1 doctests failed
sage -t -long devel/sage/sage/stats/basic_stats.py # 2 doctests failed
</pre></li></ul><ul><li>The documentation of content for multivariate polynomials says "Returns the content of this polynomial.
Here, we define content as the gcd of the coefficients in the base ring."
Your changed doctest (1 becomes 2) is correct, but perhaps it would be more informative to
add <code>f.content().parent()</code>, and to give an example over another field. (Just a suggestion
if you are editing the code anyway. Otherwise, don't bother.)
More important:
<pre class="wiki"># Since trac ticket #10771, the gcd in QQ restricts to the
# gcd in ZZ.
</pre>I don't think this comment is needed in this part of the code. If you want to include it, it would better fit in the doctest a few lines above it.
And I think junk like the following should not be in the code at all: <code> #,integer=self.parent() is ZZ) </code>
</li></ul>
TicketSimonKingTue, 15 Feb 2011 13:31:45 GMT
https://trac.sagemath.org/ticket/10771#comment:14
https://trac.sagemath.org/ticket/10771#comment:14
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:13" title="Comment 13">mstreng</a>:
</p>
<blockquote class="citation">
<ul><li>Did you run a full doctest? I'm getting doctest failures that may have something to do with your patch, as they probably simplify fractions somewhere deep inside in some way. Or my installation is messed up again.
<pre class="wiki">sage -t -long devel/sage/sage/symbolic/expression.pyx # 1 doctests failed
sage -t -long devel/sage/sage/symbolic/integration/integral.py # 1 doctests failed
sage -t -long devel/sage/sage/stats/basic_stats.py # 2 doctests failed
</pre></li></ul></blockquote>
<p>
Hum. Apparently I did not run these tests. The problem is that I currently work on various patches, frequently switch between them, and it may happen that I test one of them but forget the other.
</p>
<p>
Anyway, the failure in "expression.pyx" seems to be related with the patch. I'll try to understand what is happening.
</p>
<blockquote class="citation">
<ul><li>The documentation of content for multivariate polynomials says "Returns the content of this polynomial.
Here, we define content as the gcd of the coefficients in the base ring."
Your changed doctest (1 becomes 2) is correct, but perhaps it would be more informative to
add <code>f.content().parent()</code>, and to give an example over another field. (Just a suggestion
if you are editing the code anyway. Otherwise, don't bother.)
</li></ul></blockquote>
<p>
OK.
</p>
<blockquote class="citation">
<blockquote>
<p>
More important:
</p>
<pre class="wiki"># Since trac ticket #10771, the gcd in QQ restricts to the
# gcd in ZZ.
</pre><p>
I don't think this comment is needed in this part of the code. If you want to include it, it would better fit in the doctest a few lines above it.
</p>
</blockquote>
</blockquote>
<p>
I thought of it as a comment to people who know the original code. It could be that it is a method that uses gcd only internally, so that commenting in the code seems better than in the documentation.
</p>
<blockquote class="citation">
<blockquote>
<p>
And I think junk like the following should not be in the code at all: <code> #,integer=self.parent() is ZZ) </code>
</p>
</blockquote>
</blockquote>
<p>
OK, that was an artefact of editing.
</p>
<p>
But I think most urgent are the doctest failures. It seems related to gcd for fields that are not fraction fields (such as <code>RR</code>). That would also explain why all text passed with the original version of my patch, whereas the new version fails.
</p>
<p>
Cheers,
Simon
</p>
TicketSimonKingTue, 15 Feb 2011 13:38:29 GMT
https://trac.sagemath.org/ticket/10771#comment:15
https://trac.sagemath.org/ticket/10771#comment:15
<p>
Yep, I understand where the first error comes from.
</p>
<p>
The original gcd codes says:
</p>
<pre class="wiki"> if b is not None:
if hasattr(a, "gcd"):
return a.gcd(b, **kwargs)
else:
try:
return ZZ(a).gcd(ZZ(b))
except TypeError:
raise TypeError, "unable to find gcd of %s and %s"%(a,b)
</pre><p>
Moreover, elements of <code>RR</code> did not have a gcd method. Hence, it was tried to coerce them into ZZ, and so we had
</p>
<pre class="wiki">sage: gcd(3.0,6.0)
3
</pre><p>
But with my patch, <em>all</em> field elements (including elements of <code>RR</code>) have a gcd method, so, a conversion to <code>ZZ</code> is not attempted.
</p>
<p>
I am not sure yet how I would solve it: Change the code of `sage.symbolic.expression"? Change the gcd implemented for fields that are no fraction fields, such that a special case is made for inexact fields?
</p>
TicketSimonKingTue, 15 Feb 2011 13:59:36 GMT
https://trac.sagemath.org/ticket/10771#comment:16
https://trac.sagemath.org/ticket/10771#comment:16
<p>
I suggest to do the following, which would generalize the current attempt to convert stuff into <code>ZZ</code>:
</p>
<pre class="wiki"> For inexact fields, evaluation in the prime subfield is attempted::
sage: lcm(15.2,12.0); lcm(15.2,12.0).parent()
228
Rational Field
sage: RR(76/5)
15.2000000000000
sage: lcm(76/5,12)
228
If this fails, we resort to the default we see above::
sage: lcm(6.0*CC.0,8*CC.0); lcm(6.0*CC.0,8*CC.0).parent()
1.00000000000000
Complex Field with 53 bits of precision
</pre><p>
Similarly for gcd.
</p>
<p>
In that way, the first of the three doctest failures vanishes. But the other two remain problems.
</p>
TicketSimonKingTue, 15 Feb 2011 14:04:27 GMT
https://trac.sagemath.org/ticket/10771#comment:17
https://trac.sagemath.org/ticket/10771#comment:17
<p>
The second failure can be triggered as follows:
</p>
<pre class="wiki">sage: F(x) = 1/sqrt(2*pi*1^2)*exp(-1/(2*1^2)*(x-0)^2)
sage: G(x) = 1/sqrt(2*pi*n(1)^2)*exp(-1/(2*n(1)^2)*(x-n(0))^2)
sage: ((F(x)-G(x))^2).expand()
/mnt/local/king/SAGE/sage-4.6.2.alpha4/local/bin/sage-sage: Zeile 300: 18366 Speicherzugriffsfehler sage-ipython "$@" -i
</pre><p>
Segmentation fault while expanding. I could imagine that <code>gcd==1</code> results in an infinite loop.
</p>
TicketSimonKingTue, 15 Feb 2011 14:05:46 GMT
https://trac.sagemath.org/ticket/10771#comment:18
https://trac.sagemath.org/ticket/10771#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:17" title="Comment 17">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Segmentation fault while expanding. I could imagine that <code>gcd==1</code> results in an infinite loop.
</p>
</blockquote>
<p>
Ouch, it is worse: It already occurs <em>before</em> the expansion!
</p>
TicketSimonKingTue, 15 Feb 2011 14:34:05 GMT
https://trac.sagemath.org/ticket/10771#comment:19
https://trac.sagemath.org/ticket/10771#comment:19
<p>
The third error boils down to:
</p>
<p>
Without patch
</p>
<pre class="wiki">sage: x = -sqrt(2)-1/5*I
sage: x*x
1/25*(5*sqrt(2) + I)^2
</pre><p>
With patch
</p>
<pre class="wiki">sage: x = -sqrt(2)-1/5*I
sage: x*x
1/25*(-5*sqrt(2) - I)^2
</pre><p>
The segfault in the second doctest problem also comes from multiplication of symbolic expressions. And the example above indicates that it is related with pulling common factors out of a list of expressions.
</p>
TicketkcrismanWed, 16 Feb 2011 21:21:22 GMT
https://trac.sagemath.org/ticket/10771#comment:20
https://trac.sagemath.org/ticket/10771#comment:20
<p>
Just FYI, <a class="new ticket" href="https://trac.sagemath.org/ticket/8111" title="defect: gcd of rationals is trouble (new)">#8111</a> is possibly also related.
</p>
TicketSimonKingThu, 17 Feb 2011 13:22:33 GMTstatus changed; cc set
https://trac.sagemath.org/ticket/10771#comment:21
https://trac.sagemath.org/ticket/10771#comment:21
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>cc</strong>
<em>burcin</em> added
</li>
</ul>
<p>
With the new patch, all tests pass for me. The behaviour is now (mutatis mutandis for lcm):
</p>
<ul><li>If <code>ZZ</code> is a subring of a field F that is not a fraction field then <code>a.gcd(b)</code> first tests if a and b can be converted into <code>ZZ</code>. If it is the case, their gcd in <code>ZZ</code> <em>as element of <code>ZZ</code></em> is returned. This is not nice, but it is compatible with the "unpatched" behaviour of Sage and is assumed in some other parts of Sage:
<pre class="wiki"># unpatched!
sage: gcd(RR(1),RR(1))
1
sage: _.parent()
Integer Ring
</pre></li></ul><ul><li>If <code>ZZ</code> is no subring of F or conversion fails then <code>a.gcd(b)</code> returns <code>F(0)</code> or <code>F(1)</code>. This is, e.g., the case for <code>gcd(CC(2),CC(2))</code>, since <code>CC(2)</code> can not be converted into <code>ZZ</code>.
</li></ul><ul><li>a.gcd(b) only raises an error if <code>b</code> can not be converted into <code>a.parent()</code>.
</li></ul><ul><li>gcd and lcm in fraction fields of a unique factorization domain R restrict to R, and moreover they satisfy <code>gcd(a,b)*lcm(a,b)==a*b</code> up to multiplication with a unit of R.
</li></ul><p>
There is a price to pay, and that's why I put Burcin on Cc:
</p>
<p>
The distribution of minus signs in symbolic expression changes, apparently since now gcd raises no errors in examples like the following:
</p>
<pre class="wiki">sage: (I - 1/3*sqrt(2))^2
1/9*(-sqrt(2) + 3*I)^2
# Without patch, we would get
# 1/9*(sqrt(2) - 3*I)^2
</pre><p>
The question is whether a changed minus sign distribution is such a serious thing.
</p>
TicketSimonKingThu, 17 Feb 2011 13:46:46 GMT
https://trac.sagemath.org/ticket/10771#comment:22
https://trac.sagemath.org/ticket/10771#comment:22
<p>
Hi Marco,
</p>
<p>
sorry, I forgot to explain how I addressed your complaints:
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:10" title="Comment 10">mstreng</a>:
</p>
<blockquote class="citation">
<p>
New complaints, sorry.
</p>
<ul><li>I don't really like this:
<pre class="wiki">sage: M = Matrix(GF(3),2,2,[1,2,0,1]); M
[1 2]
[0 1]
sage: GF(2)(1).lcm(M)
1
</pre>I think you should first try to coerce other to self.parent() in gcd and lcm for Field elements.
</li></ul></blockquote>
<p>
Yep, that was a mistake. We now have:
</p>
<pre class="wiki">sage: M = Matrix(GF(3),2,2,[1,2,0,1]); M
[1 2]
[0 1]
sage: GF(2)(1).lcm(M)
---------------------------------------------------------------------------
ArithmeticError Traceback (most recent call last)
...
ArithmeticError: The second argument can not be interpreted in the parent of the first argument. Can't compute the lcm
sage: GF(2)(1).gcd(M)
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (4, 0))
---------------------------------------------------------------------------
ArithmeticError Traceback (most recent call last)
...
ArithmeticError: The second argument can not be interpreted in the parent of the first argument. Can't compute the gcd
</pre><p>
</p>
<blockquote class="citation">
<ul><li>Line 64 of quotient_fields.py "<code>both GCD and LCM, it is possible to be a bit more specific</code>"
uses capital letters GCD and LCM, while the methods that it (quasi-)refers to are lower case:
</li></ul></blockquote>
<p>
I changed the doc string to lower case.
</p>
<blockquote class="citation">
<ul><li>Next line: "define the GCD uniquely up to a unit in the base ring". Write "of" instead of "in" so that it is clear that you are not saying that the GCD is in the base ring.
</li></ul></blockquote>
<p>
Done.
</p>
<p>
</p>
<blockquote class="citation">
<ul><li>Your tests with <code>R.<q> = ZZ.extension(x^2+5)</code> is unrelated to fraction field:
</li></ul></blockquote>
<p>
You are right. My intention was to <em>start</em> with <code>R</code>, show that there is no gcd in R, and then expose what happens in <code>Frac(R)</code>. The test is changed accordingly.
</p>
<p>
Best regards,
</p>
<p>
Simon
</p>
TicketkcrismanThu, 17 Feb 2011 14:04:54 GMT
https://trac.sagemath.org/ticket/10771#comment:23
https://trac.sagemath.org/ticket/10771#comment:23
<blockquote class="citation">
<p>
There is a price to pay, and that's why I put Burcin on Cc:
</p>
<p>
The distribution of minus signs in symbolic expression changes, apparently since now gcd raises no errors in examples like the following:
</p>
<pre class="wiki">sage: (I - 1/3*sqrt(2))^2
1/9*(-sqrt(2) + 3*I)^2
# Without patch, we would get
# 1/9*(sqrt(2) - 3*I)^2
</pre><p>
The question is whether a changed minus sign distribution is such a serious thing.
</p>
</blockquote>
<p>
What happens with the already existing doctests? That would be what I would be most concerned about. Also, is the infinite loop now gone? IIRC you had fixed that somehow.
</p>
<p>
Really, in some sense the new version is 'better' because it keeps the minus where the user put it. Do <code>(-I - 1/3*sqrt(2))^2</code> and <code>(I + 1/3*sqrt(2))^2</code> do anything different from before?
</p>
TicketSimonKingThu, 17 Feb 2011 14:21:16 GMT
https://trac.sagemath.org/ticket/10771#comment:24
https://trac.sagemath.org/ticket/10771#comment:24
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:23" title="Comment 23">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
What happens with the already existing doctests? That would be what I would be most concerned about.
</p>
</blockquote>
<p>
There are two tests (really in fact only one) in <code>sage.stats</code> where I had to change the expected output. The old and new output are equivalent:
</p>
<pre class="wiki">sage: std([I, sqrt(2), 3/5]) # result with patch
sqrt(1/450*(-5*sqrt(2) + 10*I - 3)^2 + 1/450*(-5*sqrt(2) - 5*I + 6)^2 + 1/450*(10*sqrt(2) - 5*I - 3)^2)
# Without patch, we had sqrt(1/450*(5*sqrt(2) + 5*I - 6)^2 + 1/450*(5*sqrt(2) - 10*I + 3)^2 + 1/450*(10*sqrt(2) - 5*I - 3)^2)
# Testing equality:
sage: bool(sqrt(1/450*(5*sqrt(2) + 5*I - 6)^2 + 1/450*(5*sqrt(2) - 10*I + 3)^2 + 1/450*(10*sqrt(2) - 5*I - 3)^2) == std([I, sqrt(2), 3/5]))
True
</pre><p>
There is essentially the same happening with <code>variance([I, sqrt(2), 3/5])</code>.
</p>
<blockquote class="citation">
<p>
Also, is the infinite loop now gone? IIRC you had fixed that somehow.
</p>
</blockquote>
<p>
Yep. The problem was that in an old version of my patch I would not try conversion to <code>ZZ</code> but conversion to the prime subfield. A real number with finitely many digits can of course <em>always</em> be converted into <code>RR.prime_subfield()</code>, which is <code>QQ</code>. So, I guess that the segfault came from rationals with very large numerator and denominator. Here is the proof that the segfault has gone:
</p>
<pre class="wiki">sage: F(x) = 1/sqrt(2*pi*1^2)*exp(-1/(2*1^2)*(x-0)^2)
sage: G(x) = 1/sqrt(2*pi*n(1)^2)*exp(-1/(2*n(1)^2)*(x-n(0))^2)
sage: (F-G)**2
x |--> 1/4*(sqrt(2)*e^(-1/2*x^2)/sqrt(pi) - 1.41421356237309*e^(-0.500000000000000*x^2)/sqrt(pi))^2
</pre><p>
which coincides with the answer of unpatched Sage.
</p>
<blockquote class="citation">
<p>
Really, in some sense the new version is 'better' because it keeps the minus where the user put it. Do <code>(-I - 1/3*sqrt(2))^2</code> and <code>(I + 1/3*sqrt(2))^2</code> do anything different from before?
</p>
</blockquote>
<p>
They do.
</p>
<pre class="wiki">sage: (-I - 1/3*sqrt(2))^2
1/9*(-sqrt(2) - 3*I)^2
# Was: 1/9*(sqrt(2) + 3*I)^2
sage: (I + 1/3*sqrt(2))^2
1/9*(sqrt(2) + 3*I)^2
# Was: 1/9*(sqrt(2) + 3*I)^2
</pre><p>
Really I don't understand where that comes from. Namely, as far as I know, the sign of gcd or lcm did not change. The only difference in gcd/lcm examples involving <code>I</code> and <code>sqrt(2)</code> is the fact that without my patch an error would be raised.
</p>
TicketkcrismanThu, 17 Feb 2011 14:47:09 GMT
https://trac.sagemath.org/ticket/10771#comment:25
https://trac.sagemath.org/ticket/10771#comment:25
<blockquote class="citation">
<blockquote class="citation">
<p>
Really, in some sense the new version is 'better' because it keeps the minus where the user put it. Do <code>(-I - 1/3*sqrt(2))^2</code> and <code>(I + 1/3*sqrt(2))^2</code> do anything different from before?
</p>
</blockquote>
<p>
They do.
sage: (-I - 1/3*sqrt(2))<sup>2
1/9*(-sqrt(2) - 3*I)</sup>2
# Was: 1/9*(sqrt(2) + 3*I)<sup>2
</sup></p>
</blockquote>
<p>
Okay, that's what I figured would happen.
</p>
<blockquote class="citation">
<p>
sage: (I + 1/3*sqrt(2))<sup>2
1/9*(sqrt(2) + 3*I)</sup>2
# Was: 1/9*(sqrt(2) + 3*I)<sup>2
</sup></p>
</blockquote>
<p>
Okay, that is the same as before in any case.
</p>
<blockquote class="citation">
<p>
Really I don't understand where that comes from. Namely, as far as I know, the sign of gcd or lcm did not change. The only difference in gcd/lcm examples involving <code>I</code> and <code>sqrt(2)</code> is the fact that without my patch an error would be raised.
</p>
</blockquote>
<p>
Yes,
</p>
<pre class="wiki">sage: gcd(I + 1/3*sqrt(2),I + 1/3*sqrt(2))
---------------------------------------------------------------------------
RuntimeError Traceback (most recent call last)
</pre><p>
Interesting. Like I said earlier, Sage now sends such symbolic things to <a class="missing wiki">Pynac/Ginac?</a>, which however needs to use Sage again in order to do certain calculations like this (Mike Hansen explained it well in your thread on sage-devel). So apparently in the past it decided to factor out a -1 in certain cases based on some gcd - maybe just a default of 1 or something - whereas now it does not, due to the new gcd. What does
</p>
<pre class="wiki">sage: gcd(I + 1/3*sqrt(2),I + 1/3*sqrt(2))
</pre><p>
do after the patch? What happens with this?
</p>
<pre class="wiki">sage: gcd(4,2+2*I*sqrt(3))
</pre><p>
Notice that I am purposely taking two things NOT in a polynomial or extension thing, where it can be calculated whether it's a UFD. In the symbolic ring, who knows what's "right"?
</p>
TicketdsmThu, 17 Feb 2011 15:19:56 GMT
https://trac.sagemath.org/ticket/10771#comment:26
https://trac.sagemath.org/ticket/10771#comment:26
<p>
I think I've figured out hg queues now, so I can actually try out the patch! [No guarantees, though.] Was fuzz-testing it and noticed the following:
</p>
<pre class="wiki">sage: var("x")
x
sage: a = 1/3*x**0
sage: b = 2/5*x**0
sage: type(a)
<type 'sage.symbolic.expression.Expression'>
sage: gcd(a,b)
1/15
sage: lcm(a,b)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
/Applications/sage_devel/devel/sage-main/<ipython console> in <module>()
/Applications/sage_devel/local/lib/python2.6/site-packages/sage/rings/arith.pyc in lcm(a, b)
1562 return ZZ(a).lcm(ZZ(b))
1563 except TypeError:
-> 1564 raise TypeError, "unable to find lcm of %s and %s"%(a,b)
1565 return LCM(b)
1566
TypeError: unable to find lcm of 1/3 and 2/5
</pre><p>
All bets are off in SR, but there is an Expression.gcd (no .lcm, strangely). Now that QQ reduces to ZZ, maybe switch the attempt to try ZZ to QQ, so that rationals Just Work(tm)?
</p>
<p>
This surprised me a little too:
</p>
<pre class="wiki">sage: R.<x>=QQ[]
sage:
sage: a = 1/3*x**0
sage: b = 2/5*x**0
sage: type(a)
<type 'sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint'>
sage: gcd(a,b), lcm(a,b)
(1, 1)
sage:
sage: a = (1*x**0)/(3*x**0)
sage: b = (2*x**0)/(5*x**0)
sage: type(a)
<class 'sage.rings.fraction_field_element.FractionFieldElement_1poly_field'>
sage: gcd(a,b), lcm(a,b)
(1, 1)
</pre><p>
given that the seemingly harder cases
</p>
<pre class="wiki">sage: a = 1/3+x
sage: b = 2/5+x
sage: type(a)
<type 'sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint'>
sage: gcd(a,b), lcm(a,b), a*b-gcd(a,b)*lcm(a,b)
(1, x^2 + 11/15*x + 2/15, 0)
sage:
sage: a = (1*x**0)/(3*x**0)+x
sage: b = (2*x**0)/(5*x**0)+x
sage: type(a)
<class 'sage.rings.fraction_field_element.FractionFieldElement_1poly_field'>
sage: gcd(a,b), lcm(a,b), a*b-gcd(a,b)*lcm(a,b)
(1, x^2 + 11/15*x + 2/15, 0)
</pre><p>
behave a little nicer. Is it possible to recover the rational results?
</p>
TicketdsmThu, 17 Feb 2011 15:53:32 GMT
https://trac.sagemath.org/ticket/10771#comment:27
https://trac.sagemath.org/ticket/10771#comment:27
<p>
Ah, I just read the comments in <a class="new ticket" href="https://trac.sagemath.org/ticket/8111" title="defect: gcd of rationals is trouble (new)">#8111</a> (re: having to dive into flint). Well, at least changing ZZ to QQ in rings.arith.lcm would be straightforward.
</p>
TicketSimonKingThu, 17 Feb 2011 16:21:26 GMT
https://trac.sagemath.org/ticket/10771#comment:28
https://trac.sagemath.org/ticket/10771#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:27" title="Comment 27">dsm</a>:
</p>
<blockquote class="citation">
<p>
Ah, I just read the comments in <a class="new ticket" href="https://trac.sagemath.org/ticket/8111" title="defect: gcd of rationals is trouble (new)">#8111</a> (re: having to dive into flint). Well, at least changing ZZ to QQ in rings.arith.lcm would be straightforward.
</p>
</blockquote>
<p>
It could be that it is not straight forward. If my analysis of the segfault occuring with a previous version of my patch is correct, changing <code>ZZ</code> to <code>QQ</code> could very well trigger the segfault again.
</p>
TicketdsmFri, 25 Feb 2011 12:04:18 GMTowner changed
https://trac.sagemath.org/ticket/10771#comment:29
https://trac.sagemath.org/ticket/10771#comment:29
<ul>
<li><strong>owner</strong>
changed from <em>AlexGhitza</em> to <em>dsm</em>
</li>
</ul>
<p>
Well, even after changing ZZ to QQ there everything seems to work as expected for me (no segfaults, a quick subset of tests all passed; I'll try testall long later and look for more crashers), and if it did explode it sort of feels like it'd be due to a bug somewhere else that it revealed.. but that can wait for another ticket later.
</p>
<p>
The real meat is already in your patch, and I'm already using it, so I'm very grateful for the work, and hope someone gives the okay soon! <code>:^)</code>
</p>
TicketdsmWed, 02 Mar 2011 05:23:34 GMTowner changed
https://trac.sagemath.org/ticket/10771#comment:30
https://trac.sagemath.org/ticket/10771#comment:30
<ul>
<li><strong>owner</strong>
changed from <em>dsm</em> to <em>AlexGhitza</em>
</li>
</ul>
TicketSimonKingWed, 02 Mar 2011 06:40:27 GMT
https://trac.sagemath.org/ticket/10771#comment:31
https://trac.sagemath.org/ticket/10771#comment:31
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:29" title="Comment 29">dsm</a>:
</p>
<blockquote class="citation">
<p>
Well, even after changing ZZ to QQ there everything seems to work as expected for me (no segfaults, a quick subset of tests all passed; I'll try testall long later and look for more crashers), and if it did explode it sort of feels like it'd be due to a bug somewhere else that it revealed.. but that can wait for another ticket later.
</p>
</blockquote>
<p>
What came out of the tests?
</p>
<blockquote class="citation">
<p>
The real meat is already in your patch, and I'm already using it, so I'm very grateful for the work, and hope someone gives the okay soon! <code>:^)</code>
</p>
</blockquote>
<p>
You (after finishing the tests, of course)?? Or you could post a patch for changing <code>ZZ</code> to <code>QQ</code> in <code>rings.arith.lcm</code>, and perhaps I can have a look on it? If I remember correctly, it is ok if you are reviewer for my patch and I am reviewer for your patch.
</p>
TicketdsmWed, 02 Mar 2011 12:29:23 GMT
https://trac.sagemath.org/ticket/10771#comment:32
https://trac.sagemath.org/ticket/10771#comment:32
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:31" title="Comment 31">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
What came out of the tests?
</p>
</blockquote>
<p>
I seem to recall it passed just fine but I can't find the output now <code>:-(</code> so I'm rerunning them (takes me about ~3h).
</p>
<blockquote class="citation">
<p>
You (after finishing the tests, of course)??
</p>
</blockquote>
<p>
Following was's recommendations (<a class="ext-link" href="http://ask.sagemath.org/question/31/should-i-_really_-review-trac-tickets"><span class="icon"></span>http://ask.sagemath.org/question/31/should-i-_really_-review-trac-tickets</a>) I don't think I've got enough experience with the Sage codebase yet to set something to positive review; I'm still finding my way around.
</p>
TicketmariahWed, 22 Jun 2011 13:04:34 GMTstatus, reviewer changed
https://trac.sagemath.org/ticket/10771#comment:33
https://trac.sagemath.org/ticket/10771#comment:33
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
changed from <em>Marco Streng</em> to <em>Marco Streng, Mariah Lenox</em>
</li>
</ul>
<p>
Applied patch to sage-4.7.1.alpha2, did 'sage -b' followed by 'make testlong'. All tests passed. Positive review!
</p>
TicketdsmWed, 22 Jun 2011 13:18:12 GMT
https://trac.sagemath.org/ticket/10771#comment:34
https://trac.sagemath.org/ticket/10771#comment:34
<p>
<Mr Burns> Excellent. </Burns> Many thanks to Simon for the work!
</p>
TicketjdemeyerWed, 22 Jun 2011 17:38:00 GMTtype, milestone changed
https://trac.sagemath.org/ticket/10771#comment:35
https://trac.sagemath.org/ticket/10771#comment:35
<ul>
<li><strong>type</strong>
changed from <em>defect</em> to <em>enhancement</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.7.1</em> to <em>sage-4.7.2</em>
</li>
</ul>
TicketjdemeyerSun, 26 Jun 2011 18:27:10 GMTstatus changed
https://trac.sagemath.org/ticket/10771#comment:36
https://trac.sagemath.org/ticket/10771#comment:36
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Could you please change the commit message such that the lines are not so long? You should wrap long lines but make sure the first still makes sense by itself, that will appear in <code>hg log</code>.
</p>
TicketSimonKingSun, 26 Jun 2011 19:03:51 GMTattachment set
https://trac.sagemath.org/ticket/10771
https://trac.sagemath.org/ticket/10771
<ul>
<li><strong>attachment</strong>
set to <em>trac10771-lcm_gcd_fraction_fields.patch</em>
</li>
</ul>
<p>
Implement gcd and lcm for general fields, with the special case of the fraction field of a unique factorization domain
</p>
TicketSimonKingSun, 26 Jun 2011 19:06:56 GMTstatus changed
https://trac.sagemath.org/ticket/10771#comment:37
https://trac.sagemath.org/ticket/10771#comment:37
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10771#comment:36" title="Comment 36">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
Could you please change the commit message such that the lines are not so long? You should wrap long lines but make sure the first still makes sense by itself, that will appear in <code>hg log</code>.
</p>
</blockquote>
<p>
Admittedly the log line was "a bit" two long. It is now spread over three lines. I hope it is fine now.
</p>
TicketjdemeyerFri, 22 Jul 2011 12:50:17 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10771#comment:38
https://trac.sagemath.org/ticket/10771#comment:38
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.2.alpha0</em>
</li>
</ul>
Ticket