Sage: Ticket #17008: Give affine schemes unique representation (needed for elliptic curves and forking)
https://trac.sagemath.org/ticket/17008
<p>
The following used to work in Sage, but does not anymore.
</p>
<p>
Define
</p>
<pre class="wiki">@fork
def compute_E():
E = EllipticCurve([2,3])
p = E(3,6,1)
return p
</pre><p>
Then
</p>
<pre class="wiki">sage: p = compute_E()
sage: 2*p
---------------------------------------------------------------------------
RuntimeError Traceback (most recent call last)
<ipython-input-5-a9d49bd49c00> in <module>()
----> 1 Integer(2)*p
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.RingElement.__mul__ (build/cythonized/sage/structure/element.c:16558)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:7839)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.get_action (build/cythonized/sage/structure/coerce.c:13470)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.discover_action (build/cythonized/sage/structure/coerce.c:14918)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.get_action (build/cythonized/sage/structure/parent.c:20486)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_action (build/cythonized/sage/structure/parent.c:21783)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce_actions.so in sage.structure.coerce_actions.IntegerMulAction.__init__ (build/cythonized/sage/structure/coerce_actions.c:8432)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.ModuleElement.__add__ (build/cythonized/sage/structure/element.c:11295)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (build/cythonized/sage/structure/coerce.c:7904)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.canonical_coercion (build/cythonized/sage/structure/coerce.c:9490)()
/home/pcl337b/jkeitel/sage/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps._coercion_error (build/cythonized/sage/structure/coerce.c:16376)()
RuntimeError: There is a bug in the coercion code in Sage.
Both x (=(3 : 6 : 1)) and y (=(3 : -6 : 1)) are supposed to have identical parents but they don't.
In fact, x has parent 'Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field'
whereas y has parent 'Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field'
Original elements (3 : 6 : 1) (parent Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field) and (3 : -6 : 1) (parent Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field) and maps
<type 'NoneType'> None
<type 'sage.structure.coerce_maps.DefaultConvertMap_unique'> (map internal to coercion system -- copy before use)
Conversion map:
From: Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field
To: Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field
sage:
</pre><p>
and
</p>
<pre class="wiki">sage: (-p).parent() is p.parent()
sage: False
</pre><p>
Note that this depends crucially on the <code>@fork</code>, without it things work just fine. Intuitively, I suppose that since E is created in the forked process, the main process where I'm negating p does not know about E and therefore creates a new parent. However, I'm not sure how to fix this.
</p>
<p>
Since this definitely worked a few months ago, my gut says that it might have something to do with the changes introduced in <a class="closed ticket" href="https://trac.sagemath.org/ticket/11474" title="defect: Elliptic curves should be unique parent structures (closed: fixed)">#11474</a> and I've taken the liberty to include some of you who worked on this. I hope that's okay for you.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/17008
Trac 1.1.6nbruinThu, 18 Sep 2014 15:57:13 GMT
https://trac.sagemath.org/ticket/17008#comment:1
https://trac.sagemath.org/ticket/17008#comment:1
<p>
It looks like the problem is in the point sets, not the elliptic curves:
</p>
<pre class="wiki">sage: A=parent(p)
sage: B=parent(-p)
sage: id(A)==id(B)
False
sage: A._codomain
Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field
sage: id(A._codomain)==id(B._codomain)
True
</pre><p>
In fact, even the "Spectrum of Ration Field" is multiply instantiated (it arises as domain of the point set. Do we really get any mileage out of carrying around the formal homset stuff?
</p>
<pre class="wiki">sage: A._domain
Spectrum of Rational Field
sage: id(A._domain)==id(B._domain)
False
</pre><p>
We can create nonidentical-but-equal pointsets without fork:
</p>
<pre class="wiki">sage: E=EllipticCurve([2,3])
sage: A=E.point_set()
sage: B=E.point_set(QQ)
sage: parent(A(3,6,1)+B(3,6,1)) is A
True
sage: parent(B(3,6,1)+B(3,6,1)) is A
True
sage: A is B
False
sage: A==B
True
</pre>
TicketcremonaThu, 18 Sep 2014 16:29:17 GMT
https://trac.sagemath.org/ticket/17008#comment:2
https://trac.sagemath.org/ticket/17008#comment:2
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:1" title="Comment 1">nbruin</a>:
</p>
<blockquote>
<p>
Do we really get any mileage out of carrying around the formal homset stuff?
</p>
</blockquote>
<p>
I use the elliptic curve & point stuff in Sage all the time, and never have any use for that. I don't use point-sets at all.
</p>
TicketnbruinThu, 18 Sep 2014 19:01:46 GMT
https://trac.sagemath.org/ticket/17008#comment:3
https://trac.sagemath.org/ticket/17008#comment:3
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:2" title="Comment 2">cremona</a>:
</p>
<blockquote class="citation">
<p>
I use the elliptic curve & point stuff in Sage all the time, and never have any use for that. I don't use point-sets at all.
</p>
</blockquote>
<p>
Not explicitly perhaps, but it does get created (unless "parent" triggers something lazy here):
</p>
<pre class="wiki">sage: parent(EllipticCurve([2,3])(3,6,1))
Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field
</pre><p>
which is why I'm slightly concerned about "carrying around" this stuff. It may be OK: The point set should obviously carry around the field (in the form of the <code>._domain</code> attribute) and it may be that "Spectrum of Rational Field" is a sufficiently lightweight/permanent wrapper that it doesn't hurt too much.
</p>
TicketnbruinFri, 19 Sep 2014 00:40:19 GMT
https://trac.sagemath.org/ticket/17008#comment:4
https://trac.sagemath.org/ticket/17008#comment:4
<p>
It looks to me the problem with point sets indeed explains the problem:
point_homset is a cached_method, but the "fork" construction somehow circumvents the cache:
</p>
<pre class="wiki">sage: p=compute_E()
sage: p.parent()._codomain.point_homset.cache
{}
sage: 2*p
...
sage: p.parent()._codomain.point_homset.cache
{((None,),
()): Abelian group of points on Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field}
</pre><p>
A new pointset was created! If we avoid this, everything works fine:
</p>
<pre class="wiki">sage: p.parent()._codomain.point_homset.set_cache(parent(p))
sage: 2*p
(-23/144 : 2827/1728 : 1)
</pre><p>
This is probably explained by:
</p>
<pre class="wiki">sage: p.parent().__reduce_ex__(2)
(<class 'sage.schemes.generic.homset.SchemeHomsetFactory'>,
(Spectrum of Rational Field,
Elliptic Curve defined by y^2 = x^3 + 2*x + 3 over Rational Field,
Category of schemes over Integer Ring,
Integer Ring,
False,
True))
sage: p.parent()._codomain.__reduce_ex__(2)
(<function sage.structure.factory.generic_factory_unpickle>,
(<class 'sage.schemes.elliptic_curves.constructor.EllipticCurveFactory'>,
(6, 4, 'beta2'),
(Rational Field, (0, 0, 0, 2, 3)),
{}))
</pre><p>
so, when p gets pickled, then <code>p.parent()</code> gets pickled and as a result the elliptic curve as well, but the cache doesn't get pickled (as you can see, the elliptic curve gets created "fresh").
</p>
<p>
The problem could be solved by making point sets unique, but probably you'd have to make spec unique in the process as well; it is not at the moment:
</p>
<pre class="wiki">sage: a=Spec(QQ); b=Spec(QQ)
sage: a is b , a == b
(False, True)
</pre><p>
Incidentally, this also explains:
</p>
<pre class="wiki">sage: E=EllipticCurve([2,3])
sage: A=E.point_homset()
sage: B=E.point_homset(QQ)
sage: A is B
False
sage: [A is c for c in E.point_homset.cache.values()]
[True, False]
</pre><p>
two entries in the cache: one for each input type. However, these point sets seem a little better coordinated than the ones resulting from unpickling:
</p>
<pre class="wiki">sage: 2*A(3,6,1)
(-23/144 : 2827/1728 : 1)
sage: 2*B(3,6,1)
(-23/144 : 2827/1728 : 1)
sage: A(3,6,1)+B(3,6,1)
(-23/144 : 2827/1728 : 1)
</pre>
TicketpbruinFri, 19 Sep 2014 09:26:10 GMT
https://trac.sagemath.org/ticket/17008#comment:5
https://trac.sagemath.org/ticket/17008#comment:5
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:4" title="Comment 4">nbruin</a>:
</p>
<blockquote class="citation">
<p>
The problem could be solved by making point sets unique, but probably you'd have to make spec unique in the process as well; it is not at the moment:
</p>
<pre class="wiki">sage: a=Spec(QQ); b=Spec(QQ)
sage: a is b , a == b
(False, True)
</pre></blockquote>
<p>
Indeed. I am now experimenting with making <code>AffineScheme</code> inherit from <code>UniqueRepresentation</code>, and in fact this already gives the desired results without any special changes to point sets:
</p>
<pre class="wiki">sage: @fork
....: def compute_E():
....: E = EllipticCurve([2,3])
....: p = E(3,6,1)
....: return p
....:
sage: p = compute_E()
sage: 2*p
(-23/144 : 2827/1728 : 1)
</pre><pre class="wiki">sage: a=Spec(QQ); b=Spec(QQ)
sage: a is b , a == b
(True, True)
</pre><pre class="wiki">sage: E=EllipticCurve([2,3])
sage: A=E.point_homset()
sage: B=E.point_homset(QQ)
sage: A is B
True
</pre>
TicketSimonKingFri, 19 Sep 2014 09:32:39 GMT
https://trac.sagemath.org/ticket/17008#comment:6
https://trac.sagemath.org/ticket/17008#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:5" title="Comment 5">pbruin</a>:
</p>
<blockquote class="citation">
<p>
Indeed. I am now experimenting with making <code>AffineScheme</code> inherit from <code>UniqueRepresentation</code>, and in fact this already gives the desired results without any special changes to point sets:
</p>
</blockquote>
<p>
Isn't <code>UniqueRepresentation</code> cool? <code>:-D</code>
</p>
<p>
But as you know, it can introduce difficult to debug memory leaks in some cases...
</p>
TicketpbruinFri, 19 Sep 2014 10:16:19 GMT
https://trac.sagemath.org/ticket/17008#comment:7
https://trac.sagemath.org/ticket/17008#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:6" title="Comment 6">SimonKing</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:5" title="Comment 5">pbruin</a>:
</p>
<blockquote class="citation">
<p>
Indeed. I am now experimenting with making <code>AffineScheme</code> inherit from <code>UniqueRepresentation</code>, and in fact this already gives the desired results without any special changes to point sets:
</p>
</blockquote>
<p>
Isn't <code>UniqueRepresentation</code> cool? <code>:-D</code>
</p>
</blockquote>
<p>
It seems to work quite well, except it took me some time to figure out the following caveat:
</p>
<pre class="wiki">sage: class X(UniqueRepresentation):
....: def __init__(self, x, y=None):
....: self.x = x
....: self.y = y
....:
sage: A = X(1)
sage: B = X(1)
sage: C = X(1, None)
sage: A is B
True
sage: B is C
False
</pre><p>
Apparently it makes a difference whether the default <code>None</code> argument is explicitly given or not. I accept that this may be a technical limitation that isn't worth fixing, though.
</p>
TicketSimonKingFri, 19 Sep 2014 10:46:20 GMT
https://trac.sagemath.org/ticket/17008#comment:8
https://trac.sagemath.org/ticket/17008#comment:8
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:7" title="Comment 7">pbruin</a>:
</p>
<blockquote class="citation">
<p>
Apparently it makes a difference whether the default <code>None</code> argument is explicitly given or not. I accept that this may be a technical limitation that isn't worth fixing, though.
</p>
</blockquote>
<p>
Certainly not here. Anyway, the problem is that caching happens in the <code>__classcall__</code> method that is inherited from <code>CachedRepresentation</code>, and this is unaware of default arguments to the <code>__init__</code> method.
</p>
<p>
The point is that <code>@cached_function</code>, that is used to decorate the <code>__classcall__</code>, only takes into account the argspec of <code>__classcall__</code> (which is generic), but not the argspec of <code>__init__</code>.
</p>
<p>
Default arguments are taken into account by a so-called argument fixer. Normally, this is created using the argspec of the to-be-wrapped method (here: <code>__classcall__</code>). But it could be possible to find a mechanism to create an argument fixer using the argspec of a different method.
</p>
<p>
Problem: Suppose you have a hierarchy of classes. They all inherit the <code>__classcall__</code> from <code>CachedRepresentation</code>, but have their own <code>__init__</code> (or some just inherit it), and thus the classes should inherit different instances of the cached <code>__classcall__</code> function, each with its own argument fixer.
</p>
<p>
One may think that this could be possible to do, for example, in <code>sage.misc.classcall_metaclass.ClasscallMetaclass.__cinit__</code>: This is where the classcall is assigned to a class. But I am not sure if <code>__init__</code> is available at that point. After all, the class for which we want to inspect <code>__init__</code> is just to-be-created.
</p>
<p>
Perhaps this is worth a different ticket, perhaps not (because of the bad ration of complication and gain).
</p>
TicketpbruinFri, 19 Sep 2014 11:45:09 GMTstatus, component, summary changed; author, branch, commit set
https://trac.sagemath.org/ticket/17008#comment:9
https://trac.sagemath.org/ticket/17008#comment:9
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Peter Bruin</em>
</li>
<li><strong>component</strong>
changed from <em>elliptic curves</em> to <em>algebraic geometry</em>
</li>
<li><strong>summary</strong>
changed from <em>Unique parents for elliptic curves and forking</em> to <em>Give affine schemes unique representation (needed for elliptic curves and forking)</em>
</li>
<li><strong>branch</strong>
set to <em>u/pbruin/17008-AffineScheme_unique</em>
</li>
<li><strong>commit</strong>
set to <em>d6e59567786ab3847181186964577b3842a6e70d</em>
</li>
</ul>
TickettscrimFri, 19 Sep 2014 15:49:50 GMT
https://trac.sagemath.org/ticket/17008#comment:10
https://trac.sagemath.org/ticket/17008#comment:10
<p>
@pbruin
</p>
<p>
So what Simon is recommending (I believe) is to add a method:
</p>
<div class="wiki-code"><div class="code"><pre><span class="nd">@staticmethod</span>
<span class="k">def</span> <span class="nf">__classcall__</span><span class="p">(</span>cls<span class="p">,</span> R<span class="p">,</span> S<span class="o">=</span><span class="bp">None</span><span class="p">):</span>
<span class="c"># Do any other parsing/standardization that's needed here</span>
<span class="c"># In particular, if S is None, then set it to ZZ following</span>
<span class="c"># the __init__ of Scheme</span>
<span class="k">return</span> <span class="nb">super</span><span class="p">(</span>AffineScheme<span class="p">,</span> cls<span class="p">)</span><span class="o">.</span>__classcall__<span class="p">(</span>R<span class="p">,</span> S<span class="p">)</span>
</pre></div></div><p>
or perhaps call it <code>__classcall_private__</code> (so it doesn't get accidentally used by subclasses). It almost seems like this should go one further to make <code>Scheme</code> a <code>UniqueRepresentation</code>, but IDK how much more work that would be.
</p>
<p>
However I disagree with this change:
</p>
<div class="wiki-code"><div xmlns="http://www.w3.org/1999/xhtml" class="diff">
<ul class="entries">
<li class="entry">
<h2>
<a>src/sage/categories/homset.py</a>
</h2>
<table class="trac-diff inline" summary="Differences" cellspacing="0">
<colgroup><col class="lineno" /><col class="lineno" /><col class="content" /></colgroup>
<thead>
<tr>
<th title="File a/src/sage/categories/homset.py">
a
</th>
<th title="File b/src/sage/categories/homset.py">
b
</th>
<td><em> class Homset(Set_generic):</em> </td>
</tr>
</thead>
<tbody class="unmod">
<tr>
<th>505</th><th>505</th><td class="l"><span> sage: loads(H.dumps()) is H</span></td>
</tr><tr>
<th>506</th><th>506</th><td class="l"><span> True</span></td>
</tr><tr>
<th>507</th><th>507</th><td class="l"><span></span></td>
</tr>
</tbody><tbody class="mod">
<tr class="first">
<th>508</th><th> </th><td class="l"><span> Homsets of <del>non-unique parents are non-</del>unique as well::</span></td>
</tr>
<tr class="last">
<th> </th><th>508</th><td class="r"><span> Homsets of <ins>unique parents are </ins>unique as well::</span></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>509</th><th>509</th><td class="l"><span></span></td>
</tr><tr>
<th>510</th><th>510</th><td class="l"><span> sage: H = End(AffineSpace(2, names='x,y'))</span></td>
</tr><tr>
<th>511</th><th>511</th><td class="l"><span> sage: loads(dumps(AffineSpace(2, names='x,y'))) is AffineSpace(2, names='x,y')</span></td>
</tr>
</tbody><tbody class="rem">
<tr class="first">
<th>512</th><th> </th><td class="l"><del> False</del></td>
</tr><tr class="last">
<th>513</th><th> </th><td class="l"><del> sage: loads(dumps(AffineSpace(2, names='x,y'))) == AffineSpace(2, names='x,y')</del></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>514</th><th>512</th><td class="l"><span> True</span></td>
</tr><tr>
<th>515</th><th>513</th><td class="l"><span> sage: loads(dumps(H)) is H</span></td>
</tr>
</tbody><tbody class="rem">
<tr class="first">
<th>516</th><th> </th><td class="l"><del> False</del></td>
</tr><tr class="last">
<th>517</th><th> </th><td class="l"><del> sage: loads(dumps(H)) == H</del></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>518</th><th>514</th><td class="l"><span> True</span></td>
</tr><tr>
<th>519</th><th>515</th><td class="l"><span></span></td>
</tr><tr>
<th>520</th><th>516</th><td class="l"><span> """</span></td>
</tr>
</tbody>
</table>
</li>
</ul>
</div></div><p>
I think it would be much better to instead change it to a non-unique parent.
</p>
<p>
EDIT (clarification) - By 'it' above I meant the parent in the test.
</p>
TicketSimonKingFri, 19 Sep 2014 16:36:21 GMT
https://trac.sagemath.org/ticket/17008#comment:11
https://trac.sagemath.org/ticket/17008#comment:11
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:10" title="Comment 10">tscrim</a>:
</p>
<blockquote class="citation">
<p>
So what Simon is recommending (I believe) is to add a method:
</p>
<div class="wiki-code"><div class="code"><pre><span class="nd">@staticmethod</span>
<span class="k">def</span> <span class="nf">__classcall__</span><span class="p">(</span>cls<span class="p">,</span> R<span class="p">,</span> S<span class="o">=</span><span class="bp">None</span><span class="p">):</span>
<span class="c"># Do any other parsing/standardization that's needed here</span>
<span class="c"># In particular, if S is None, then set it to ZZ following</span>
<span class="c"># the __init__ of Scheme</span>
<span class="k">return</span> <span class="nb">super</span><span class="p">(</span>AffineScheme<span class="p">,</span> cls<span class="p">)</span><span class="o">.</span>__classcall__<span class="p">(</span>R<span class="p">,</span> S<span class="p">)</span>
</pre></div></div><p>
or perhaps call it <code>__classcall_private__</code> (so it doesn't get accidentally used by subclasses).
</p>
</blockquote>
<p>
That's the way to work around the limitation of <code>UniqueRepresentation</code> in special cases. Note that what we want here is to make <code>__classcall__</code> aware of the default arguments of <code>__init__</code> (or rather: Provide these default arguments). Hence, we <em>want</em> that this behaviour is inherited for all sub-classes that also inherit <code>__init__</code>. Therefore, <code>__classcall__</code> should be better than <code>__classcall_private__</code> in this case.
</p>
<p>
However, what I was mentioning was an approach to leverage the limitation <em>generally</em>, without the need to implement <code>__classcall__</code> or <code>__classcall_private__</code>.
</p>
TicketSimonKingFri, 19 Sep 2014 16:39:18 GMT
https://trac.sagemath.org/ticket/17008#comment:12
https://trac.sagemath.org/ticket/17008#comment:12
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:10" title="Comment 10">tscrim</a>:
</p>
<blockquote class="citation">
<p>
However I disagree with this change:
</p>
<div class="wiki-code"><div xmlns="http://www.w3.org/1999/xhtml" class="diff">
<ul class="entries">
<li class="entry">
<h2>
<a>src/sage/categories/homset.py</a>
</h2>
<table class="trac-diff inline" summary="Differences" cellspacing="0">
<colgroup><col class="lineno" /><col class="lineno" /><col class="content" /></colgroup>
<thead>
<tr>
<th title="File a/src/sage/categories/homset.py">
a
</th>
<th title="File b/src/sage/categories/homset.py">
b
</th>
<td><em> class Homset(Set_generic):</em> </td>
</tr>
</thead>
<tbody class="unmod">
<tr>
<th>505</th><th>505</th><td class="l"><span> sage: loads(H.dumps()) is H</span></td>
</tr><tr>
<th>506</th><th>506</th><td class="l"><span> True</span></td>
</tr><tr>
<th>507</th><th>507</th><td class="l"><span></span></td>
</tr>
</tbody><tbody class="mod">
<tr class="first">
<th>508</th><th> </th><td class="l"><span> Homsets of <del>non-unique parents are non-</del>unique as well::</span></td>
</tr>
<tr class="last">
<th> </th><th>508</th><td class="r"><span> Homsets of <ins>unique parents are </ins>unique as well::</span></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>509</th><th>509</th><td class="l"><span></span></td>
</tr><tr>
<th>510</th><th>510</th><td class="l"><span> sage: H = End(AffineSpace(2, names='x,y'))</span></td>
</tr><tr>
<th>511</th><th>511</th><td class="l"><span> sage: loads(dumps(AffineSpace(2, names='x,y'))) is AffineSpace(2, names='x,y')</span></td>
</tr>
</tbody><tbody class="rem">
<tr class="first">
<th>512</th><th> </th><td class="l"><del> False</del></td>
</tr><tr class="last">
<th>513</th><th> </th><td class="l"><del> sage: loads(dumps(AffineSpace(2, names='x,y'))) == AffineSpace(2, names='x,y')</del></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>514</th><th>512</th><td class="l"><span> True</span></td>
</tr><tr>
<th>515</th><th>513</th><td class="l"><span> sage: loads(dumps(H)) is H</span></td>
</tr>
</tbody><tbody class="rem">
<tr class="first">
<th>516</th><th> </th><td class="l"><del> False</del></td>
</tr><tr class="last">
<th>517</th><th> </th><td class="l"><del> sage: loads(dumps(H)) == H</del></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>518</th><th>514</th><td class="l"><span> True</span></td>
</tr><tr>
<th>519</th><th>515</th><td class="l"><span></span></td>
</tr><tr>
<th>520</th><th>516</th><td class="l"><span> """</span></td>
</tr>
</tbody>
</table>
</li>
</ul>
</div></div><p>
I think it would be much better to instead change it to a non-unique parent.
</p>
</blockquote>
<p>
Homsets are cached by <em>identity</em> of domain and codomain. Hence, if domain or codomain are non-unique parents, then homsets automatically are non-unique parents as well. However, I think it is true (and already documented, that's why I disagree with this change) that homsets of unique parents are unique parents.
</p>
TicketnbruinFri, 19 Sep 2014 17:30:55 GMT
https://trac.sagemath.org/ticket/17008#comment:13
https://trac.sagemath.org/ticket/17008#comment:13
<p>
I'd expect that caching strategies need amending if schemes move over to <code>UniqueRepresentations</code>. As we know, if one has an object <code>A</code> from which another object <code>b=B(A,...)</code> is constructed then if b is <code>UniqueRepresentation</code>, it should NOT be cached on A. That's because the global <code>UniqueRepresentation</code> cache holds a strong reference to A for the lifetime of b (i.e., A will not die before b).
</p>
<p>
If A also holds a strong reference to b (in its local cache) then b will also not die before A: they're immortal! The garbage collector does not recognize this, because the reference to A is truly global, so it's not a cycle (it's key in a <code>WeakValueDict</code>, so the reference is artificially removed should <code>b</code> be deallocated).
</p>
<p>
I haven't checked, but the fact that point sets are cached on the scheme seems likely to instantiate a scenario as above.
</p>
<p>
<code>UniqueRepresentation</code> is nice for coercion, but it's a pain for memory management. Most memory management issues in Python are alleviated by the cyclic garbage collector (CGC) (reference counting simply doesn't cut it), but the mechanisms involved in <code>UniqueRepresentation</code> can defeat the CGC. That's not pleasant.
</p>
<p>
You'd better have good reasons for condemning data structures to <code>UniqueRepresentation</code>. The original problem on this ticket is more a pickling problem. It can also be solved by amending the pickling data structures (e.g., by making point sets pickle to point set constructors rather than a generic type <code>__new__</code> and filling the <code>__dict__</code>, or by making points pickle differently)
</p>
TicketnbruinFri, 19 Sep 2014 20:21:18 GMT
https://trac.sagemath.org/ticket/17008#comment:14
https://trac.sagemath.org/ticket/17008#comment:14
<p>
It seems we're currently not leaking pointsets and elliptic curves themselves. We are leaking all kinds of other structures, though, so constructing many reductions of an elliptic curve will not be possible. It's also scary that we're creating multivariate polynomial rings (and leaking them!) just to construct a point on an elliptic curve (in fact all this junk already is created if just the elliptic curve is created).
</p>
<pre class="wiki">sage: import gc
sage: from collections import Counter
sage: pre = {id(a) for a in gc.get_objects()}
sage: for p in prime_range(5,2000):
....: k=GF(p)
....: E=EllipticCurve(k,[0,1])
....: V=E(0,1)
....:
sage: gc.collect()
2297
sage: gc.collect()
0
sage: new = Counter(str(type(c)) for c in gc.get_objects() if id(c) not in pre)
sage: for a,b in new.iteritems(): print a,b
<type 'sage.misc.cachefunc.CachedFunction'> 1
<type 'sage.misc.cachefunc.CachedMethodCallerNoArgs'> 5
<type 'frame'> 2
<class 'weakref.KeyedRef'> 8751
<type 'weakref'> 3384
<type 'set'> 1
<type 'method_descriptor'> 19
<type 'sage.misc.constant_function.ConstantFunction'> 1204
<class '_ast.comprehension'> 1
<class '_ast.Compare'> 1
<type 'sage.structure.coerce_dict.TripleDict'> 907
<class '_ast.Attribute'> 1
<class '_ast.Interactive'> 1
<class 'sage.categories.schemes.Schemes_over_base_with_category'> 1
<type 'sage.structure.coerce_actions.LeftModuleAction'> 301
<class 'sage.categories.commutative_algebras.CommutativeAlgebras_with_category'> 1
<class 'sage.rings.ideal.Ideal_pid'> 301
<type 'tuple'> 4504
<type 'sage.rings.finite_rings.integer_mod.NativeIntStruct'> 301
<class 'sage.structure.dynamic_class.DynamicMetaclass'> 14
<type 'sage.misc.cachefunc.CachedMethodCaller'> 3
<type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomialRing_libsingular'> 301
<class 'sage.rings.polynomial.term_order.TermOrder'> 602
<type 'builtin_function_or_method'> 608
<type 'module'> 3
<type 'wrapper_descriptor'> 18
<class 'sage.rings.finite_rings.finite_field_prime_modn.FiniteField_prime_modn_with_category'> 301
<class 'sage.rings.homset.RingHomset_quo_ring_with_category'> 301
<class 'sage.schemes.projective.projective_space.ProjectiveSpace_finite_field_with_category'> 1
<type 'dict'> 1576
<type 'sage.rings.polynomial.polynomial_element.PolynomialBaseringInjection'> 301
<type 'cell'> 647
<type 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular'> 1807
<type 'sage.structure.coerce_dict.MonoDictEraser'> 1814
<type 'list'> 6653
<type 'sage.misc.cachefunc.CachedMethod'> 1
<class '_ast.Call'> 5
<class 'sage.structure.dynamic_class.DynamicClasscallMetaclass'> 309
<type 'sage.structure.coerce_dict.TripleDictEraser'> 907
<type 'instancemethod'> 302
<type 'sage.rings.finite_rings.integer_mod.IntegerMod_int'> 22373
<type 'function'> 359
<class 'sage.categories.category.JoinCategory_with_category'> 5
<class '_ast.Module'> 1
<class 'sage.schemes.elliptic_curves.ell_finite_field.EllipticCurve_finite_field_with_category'> 1
<class '_ast.Name'> 9
<type 'listiterator'> 1
<type 'sage.structure.coerce_dict.MonoDict'> 1814
<type 'type'> 2
<type 'staticmethod'> 323
<class '_ast.Assign'> 1
<class 'sage.schemes.generic.scheme.AffineScheme_with_category'> 2
<type 'sage.rings.finite_rings.integer_mod.Int_to_IntegerMod'> 301
<type 'frozenset'> 2
<type 'sage.rings.finite_rings.integer_mod.Integer_to_IntegerMod'> 301
<class 'sage.categories.groupoid.Groupoid_with_category'> 301
</pre>
TicketgitSat, 20 Sep 2014 09:27:16 GMTcommit changed
https://trac.sagemath.org/ticket/17008#comment:15
https://trac.sagemath.org/ticket/17008#comment:15
<ul>
<li><strong>commit</strong>
changed from <em>d6e59567786ab3847181186964577b3842a6e70d</em> to <em>daaaee84458e4703aa5878062888404519819067</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=daaaee84458e4703aa5878062888404519819067"><span class="icon"></span>daaaee8</a></td><td><code>Trac 17008: add doctest for homsets of non-unique parents</code>
</td></tr></table>
TicketpbruinSat, 20 Sep 2014 09:35:40 GMT
https://trac.sagemath.org/ticket/17008#comment:16
https://trac.sagemath.org/ticket/17008#comment:16
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:10" title="Comment 10">tscrim</a>:
</p>
<blockquote class="citation">
<p>
It almost seems like this should go one further to make <code>Scheme</code> a <code>UniqueRepresentation</code>, but IDK how much more work that would be.
</p>
</blockquote>
<p>
I thought about this, but I think it does not make sense, because <code>Scheme</code> is just an abstract base class for general schemes, which don't usually have a set of defining data that determine them uniquely up to isomorphism. Affine schemes are uniquely determined by their coordinate ring, elliptic curves by their base ring and Weierstrass coefficients, but general schemes are just too general. We could do this for affine and projective <em>spaces</em> <strong>A</strong><sup><em>n</em></sup> and <strong>P</strong><sup><em>n</em></sup>, though.
</p>
<blockquote class="citation">
<p>
However I disagree with this change:
[...]
I think it would be much better to instead change it to a non-unique parent.
</p>
</blockquote>
<p>
Done in previous commit.
</p>
TicketpbruinSat, 20 Sep 2014 09:59:30 GMT
https://trac.sagemath.org/ticket/17008#comment:17
https://trac.sagemath.org/ticket/17008#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:14" title="Comment 14">nbruin</a>:
</p>
<blockquote class="citation">
<p>
It seems we're currently not leaking pointsets and elliptic curves themselves. We are leaking all kinds of other structures, though, so constructing many reductions of an elliptic curve will not be possible.
</p>
</blockquote>
<p>
You are right, and this is a reason why people are forced to use forking for long computations...
</p>
<blockquote class="citation">
<p>
It's also scary that we're creating multivariate polynomial rings (and leaking them!) just to construct a point on an elliptic curve (in fact all this junk already is created if just the elliptic curve is created).
</p>
</blockquote>
<p>
In this case it is probably because an elliptic curve is also a projective curve. From <code>EllipticCurve_generic</code>:
</p>
<div class="wiki-code"><div class="code"><pre><span class="k">def</span> <span class="nf">__init__</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> K<span class="p">,</span> ainvs<span class="p">):</span>
<span class="bp">self</span><span class="o">.</span>__base_ring <span class="o">=</span> K
<span class="bp">self</span><span class="o">.</span>__ainvs <span class="o">=</span> <span class="nb">tuple</span><span class="p">(</span>K<span class="p">(</span>a<span class="p">)</span> <span class="k">for</span> a <span class="ow">in</span> ainvs<span class="p">)</span>
<span class="k">if</span> <span class="bp">self</span><span class="o">.</span>discriminant<span class="p">()</span> <span class="o">==</span> <span class="mi">0</span><span class="p">:</span>
<span class="k">raise</span> <span class="ne">ArithmeticError</span><span class="p">(</span><span class="s">"invariants "</span> <span class="o">+</span> <span class="nb">str</span><span class="p">(</span>ainvs<span class="p">)</span> <span class="o">+</span> <span class="s">" define a singular curve"</span><span class="p">)</span>
PP <span class="o">=</span> projective_space<span class="o">.</span>ProjectiveSpace<span class="p">(</span><span class="mi">2</span><span class="p">,</span> K<span class="p">,</span> names<span class="o">=</span><span class="s">'xyz'</span><span class="p">);</span>
x<span class="p">,</span> y<span class="p">,</span> z <span class="o">=</span> PP<span class="o">.</span>coordinate_ring<span class="p">()</span><span class="o">.</span>gens<span class="p">()</span>
a1<span class="p">,</span> a2<span class="p">,</span> a3<span class="p">,</span> a4<span class="p">,</span> a6 <span class="o">=</span> ainvs
f <span class="o">=</span> y<span class="o">**</span><span class="mi">2</span><span class="o">*</span>z <span class="o">+</span> <span class="p">(</span>a1<span class="o">*</span>x <span class="o">+</span> a3<span class="o">*</span>z<span class="p">)</span><span class="o">*</span>y<span class="o">*</span>z \
<span class="o">-</span> <span class="p">(</span>x<span class="o">**</span><span class="mi">3</span> <span class="o">+</span> a2<span class="o">*</span>x<span class="o">**</span><span class="mi">2</span><span class="o">*</span>z <span class="o">+</span> a4<span class="o">*</span>x<span class="o">*</span>z<span class="o">**</span><span class="mi">2</span> <span class="o">+</span> a6<span class="o">*</span>z<span class="o">**</span><span class="mi">3</span><span class="p">)</span>
plane_curve<span class="o">.</span>ProjectiveCurve_generic<span class="o">.</span>__init__<span class="p">(</span><span class="bp">self</span><span class="p">,</span> PP<span class="p">,</span> f<span class="p">)</span>
</pre></div></div>
TickettscrimSat, 20 Sep 2014 18:37:46 GMT
https://trac.sagemath.org/ticket/17008#comment:18
https://trac.sagemath.org/ticket/17008#comment:18
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:16" title="Comment 16">pbruin</a>:
</p>
<blockquote class="citation">
<p>
I thought about this, but I think it does not make sense, because <code>Scheme</code> is just an abstract base class for general schemes, which don't usually have a set of defining data that determine them uniquely up to isomorphism. Affine schemes are uniquely determined by their coordinate ring, elliptic curves by their base ring and Weierstrass coefficients, but general schemes are just too general. We could do this for affine and projective <em>spaces</em> <strong>A</strong><sup><em>n</em></sup> and <strong>P</strong><sup><em>n</em></sup>, though.
</p>
</blockquote>
<p>
Ah okay (I don't know that much about the math in this area), I was looking at the code and it seems like all of input standardization (the <code>_base_*</code> attributes) could be done at initialization for very low cost (really beforehand). I think <code>==</code> doesn't check for isomorphism (at least in the base class and there are only 3 classes that I can see that directly inherit from <code>Scheme</code>), so I feel like making it into a <code>UniqueRepresentation</code> would be okay. (In fact, it seems like no <code>__eq__</code> is defined on any scheme, so it basically always returns false.) *shrugs*
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
However I disagree with this change:
[...]
I think it would be much better to instead change it to a non-unique parent.
</p>
</blockquote>
<p>
Done in previous commit.
</p>
</blockquote>
<p>
Thanks!
</p>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:14" title="Comment 14">nbruin</a>:
</p>
<blockquote class="citation">
<p>
It seems we're currently not leaking pointsets and elliptic curves themselves. We are leaking all kinds of other structures, though, so constructing many reductions of an elliptic curve will not be possible.
</p>
</blockquote>
<p>
I thought <code>UniqueRepresentation</code>'s are weakly cached, so these aren't truly leaked, but instead waiting for Sage to use up all it's memory before collecting them? I know polynomial rings use their own (weak) cache and exhibit similar output to the above.
</p>
<p>
Related question: Do we have a (documented) mechanism for freeing the <code>UniqueRepresentation</code> cache? Should we if we don't?
</p>
TicketnbruinSat, 20 Sep 2014 19:45:43 GMT
https://trac.sagemath.org/ticket/17008#comment:19
https://trac.sagemath.org/ticket/17008#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:18" title="Comment 18">tscrim</a>:
</p>
<blockquote class="citation">
<p>
I thought <code>UniqueRepresentation</code>'s are weakly cached, so these aren't truly leaked, but instead waiting for Sage to use up all it's memory before collecting them? I know polynomial rings use their own (weak) cache and exhibit similar output to the above.
</p>
</blockquote>
<p>
They are: as long as the object lives, we remember it in a <code>WeakValueDict</code>, keyed on construction parameters. When the object is deallocated, we remove the entry from the cache. The <code>WeakValueDict</code> reference by itself doesn't prevent deallocation of the object. However, the key IS strongly referenced, so if the keys somehow hold a reference to the original object, the cache is will keep the object alive. See the scenario I described in comment <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:13" title="Comment 13">13</a> or <a class="ext-link" href="https://groups.google.com/d/msg/sage-devel/q5uy_lI11jg/CB15fcRmE4cJ"><span class="icon"></span>sage-devel</a> for details.
</p>
<p>
Of course, normally one wouldn't expect that construction parameters refer to the object that is constructed from them. However, local caching strategies tend to do exactly that.
</p>
<p>
Another issue is that libsingular polynomial rings are, unfortunately, immortal. See <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>. That may well explain all the leaked objects for the particular example. The main issue is: there are plenty of things one can do with elliptic curves without explicitly referring to a plane projective model of it, so it seems a little worrying that we're always creating that.
</p>
<blockquote class="citation">
<p>
Related question: Do we have a (documented) mechanism for freeing the <code>UniqueRepresentation</code> cache? Should we if we don't?
</p>
</blockquote>
<p>
Do you mean: removing an entry from the cache even when the object still exists? That sounds like a bad idea to sanction with a formal API. However, you can do it if you want, since you can find the cache if you work at it:
</p>
<pre class="wiki">sage: A=AbelianGroup([0])
sage: k=[k for k,v in sage.structure.unique_representation.CachedRepresentation.__classcall__.cache.iteritems() if v is A][0]; k
((sage.groups.abelian_gps.abelian_group.AbelianGroup_class, (0,), 'f'), ())
sage: del sage.structure.unique_representation.CachedRepresentation.__classcall__.cache[k]
sage: B=AbelianGroup([0])
sage: A is B
False
</pre><p>
However, sage behaviour after this is likely to be questionable (so, basically no worse than before).
</p>
TickettscrimSat, 20 Sep 2014 20:17:02 GMT
https://trac.sagemath.org/ticket/17008#comment:20
https://trac.sagemath.org/ticket/17008#comment:20
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:19" title="Comment 19">nbruin</a>:
</p>
<blockquote class="citation">
<p>
They are: as long as the object lives, we remember it in a <code>WeakValueDict</code>, keyed on construction parameters. When the object is deallocated, we remove the entry from the cache. The <code>WeakValueDict</code> reference by itself doesn't prevent deallocation of the object. However, the key IS strongly referenced, so if the keys somehow hold a reference to the original object, the cache is will keep the object alive. See the scenario I described in comment <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:13" title="Comment 13">13</a> or <a class="ext-link" href="https://groups.google.com/d/msg/sage-devel/q5uy_lI11jg/CB15fcRmE4cJ"><span class="icon"></span>sage-devel</a> for details.
</p>
</blockquote>
<p>
Ahh, I see what the problem is. One of these days, this will have been explained to me enough times I'll remember it. <code>:P</code>
</p>
<blockquote class="citation">
<p>
Another issue is that libsingular polynomial rings are, unfortunately, immortal. See <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>. That may well explain all the leaked objects for the particular example. The main issue is: there are plenty of things one can do with elliptic curves without explicitly referring to a plane projective model of it, so it seems a little worrying that we're always creating that.
</p>
</blockquote>
<p>
So then would you expect <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> to help with the leak here? Is it worthwhile to try and finish <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>?
</p>
<p>
How bad do we expect the leak here to be in practice? Mainly, what's the lesser evil: the error this ticket's about or the memory leak, and should we just have a follow-up ticket if the latter is the lesser evil?
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Related question: Do we have a (documented) mechanism for freeing the <code>UniqueRepresentation</code> cache? Should we if we don't?
</p>
</blockquote>
<p>
Do you mean: removing an entry from the cache even when the object still exists? That sounds like a bad idea to sanction with a formal API. However, you can do it if you want, since you can find the cache if you work at it:
</p>
<pre class="wiki">sage: A=AbelianGroup([0])
sage: k=[k for k,v in sage.structure.unique_representation.CachedRepresentation.__classcall__.cache.iteritems() if v is A][0]; k
((sage.groups.abelian_gps.abelian_group.AbelianGroup_class, (0,), 'f'), ())
sage: del sage.structure.unique_representation.CachedRepresentation.__classcall__.cache[k]
sage: B=AbelianGroup([0])
sage: A is B
False
</pre><p>
However, sage behaviour after this is likely to be questionable (so, basically no worse than before).
</p>
</blockquote>
<p>
I was misunderstanding the weak cache as I thought they didn't get garbage collected until space was needed (even if there were no strong references). Nevermind.
</p>
TicketpbruinMon, 22 Sep 2014 08:24:39 GMT
https://trac.sagemath.org/ticket/17008#comment:21
https://trac.sagemath.org/ticket/17008#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:20" title="Comment 20">tscrim</a>:
</p>
<blockquote class="citation">
<p>
So then would you expect <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> to help with the leak here? Is it worthwhile to try and finish <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>?
</p>
</blockquote>
<p>
Just to clarify, <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> is orthogonal to this ticket; the leak occurs both with and without the attached branch. I checked that the leak goes away after applying
</p>
<div class="wiki-code"><div xmlns="http://www.w3.org/1999/xhtml" class="diff">
<ul class="entries">
<li class="entry">
<h2>
<a>src/sage/rings/polynomial/multi_polynomial_libsingular.pyx</a>
</h2>
<table class="trac-diff inline" summary="Differences" cellspacing="0">
<colgroup><col class="lineno" /><col class="lineno" /><col class="content" /></colgroup>
<thead>
<tr>
<th title="File a/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx">
a
</th>
<th title="File b/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx">
b
</th>
<td><em> from sage.misc.sage_eval import sage_eval</em> </td>
</tr>
</thead>
<tbody class="unmod">
<tr>
<th>245</th><th>245</th><td class="l"><span>import sage.libs.pari.gen</span></td>
</tr><tr>
<th>246</th><th>246</th><td class="l"><span>import polynomial_element</span></td>
</tr><tr>
<th>247</th><th>247</th><td class="l"><span></span></td>
</tr>
</tbody><tbody class="rem">
<tr class="last first">
<th>248</th><th> </th><td class="l"><del>permstore=[]</del></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>249</th><th>248</th><td class="l"><span>cdef class MPolynomialRing_libsingular(MPolynomialRing_generic):</span></td>
</tr><tr>
<th>250</th><th>249</th><td class="l"><span></span></td>
</tr><tr>
<th>251</th><th>250</th><td class="l"><span> def __cinit__(self):</span></td>
</tr>
</tbody>
<tbody class="skipped">
<tr>
<th><a href="#L367">…</a></th>
<th><a href="#L366">…</a></th>
<td><em> cdef class MPolynomialRing_libsingular(MPolynomialRing_generic):</em> </td>
</tr>
</tbody>
<tbody class="unmod">
<tr>
<th>367</th><th>366</th><td class="l"><span> from sage.rings.polynomial.polynomial_element import PolynomialBaseringInjection</span></td>
</tr><tr>
<th>368</th><th>367</th><td class="l"><span> base_inject = PolynomialBaseringInjection(base_ring, self)</span></td>
</tr><tr>
<th>369</th><th>368</th><td class="l"><span> self.register_coercion(base_inject)</span></td>
</tr>
</tbody><tbody class="rem">
<tr class="first">
<th>370</th><th> </th><td class="l"><del> #permanently store a reference to this ring until deallocation works reliably</del></td>
</tr><tr class="last">
<th>371</th><th> </th><td class="l"><del> permstore.append(self)</del></td>
</tr>
</tbody><tbody class="unmod">
<tr>
<th>372</th><th>369</th><td class="l"><span></span></td>
</tr><tr>
<th>373</th><th>370</th><td class="l"><span> def __dealloc__(self):</span></td>
</tr><tr>
<th>374</th><th>371</th><td class="l"><span> r"""</span></td>
</tr>
</tbody>
</table>
</li>
</ul>
</div></div><p>
which should be done as part of <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>. My branch does not introduce any additional leak as far as I can see.
</p>
<blockquote class="citation">
<p>
How bad do we expect the leak here to be in practice? Mainly, what's the lesser evil: the error this ticket's about or the memory leak, and should we just have a follow-up ticket if the latter is the lesser evil?
</p>
</blockquote>
<p>
It seems to me that <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> and this ticket can and should be fixed independently of each other, and there is currently no separate problem requiring another ticket.
</p>
TicketnbruinMon, 22 Sep 2014 15:52:04 GMT
https://trac.sagemath.org/ticket/17008#comment:22
https://trac.sagemath.org/ticket/17008#comment:22
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/17008#comment:21" title="Comment 21">pbruin</a>:
</p>
<blockquote class="citation">
<p>
Just to clarify, <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> is orthogonal to this ticket; the leak occurs both with and without the attached branch. I checked that the leak goes away after (removing <code>permstore</code>).
which should be done as part of <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a>. My branch does not introduce any additional leak as far as I can see.
</p>
</blockquote>
<p>
That doesn't prove that properly solving <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> would leave the memory leak in place. <code>permstore</code> was only introduced because we had examples where the deletion of polynomial rings caused memory corruption in libsingular. We don't know that <code>permstore</code> is the only link that prevents them from being deleted (and since we're not testing that, it's quite probable it's not). Memory management of polynomial rings is just broken.
</p>
<blockquote class="citation">
<p>
It seems to me that <a class="needs_work ticket" href="https://trac.sagemath.org/ticket/13447" title="defect: Make libsingular multivariate polynomial rings collectable (needs_work)">#13447</a> and this ticket can and should be fixed independently of each other, and there is currently no separate problem requiring another ticket.
</p>
</blockquote>
<p>
I do agree with that.
</p>
<p>
I do find it funny that with the fork/pickle idiom, we're basically reintroducing an extremely heavy-handed version of PARI's grepile memory management (except that pickling is an even harder problem to solve and, as we can see here, is probably broken for pretty much any sufficiently complicated data structure).
</p>
TicketvbraunSat, 25 Oct 2014 22:46:40 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/17008#comment:23
https://trac.sagemath.org/ticket/17008#comment:23
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Volker Braun</em>
</li>
</ul>
TicketvbraunSun, 26 Oct 2014 10:33:21 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/17008#comment:24
https://trac.sagemath.org/ticket/17008#comment:24
<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>branch</strong>
changed from <em>u/pbruin/17008-AffineScheme_unique</em> to <em>daaaee84458e4703aa5878062888404519819067</em>
</li>
</ul>
Ticket