-
-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Give affine schemes unique representation (needed for elliptic curves and forking) #17008
Comments
comment:1
It looks like the problem is in the point sets, not the elliptic curves:
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?
We can create nonidentical-but-equal pointsets without fork:
|
comment:2
Replying to @nbruin: 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. |
comment:3
Replying to @JohnCremona:
Not explicitly perhaps, but it does get created (unless "parent" triggers something lazy here):
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 |
comment:4
It looks to me the problem with point sets indeed explains the problem:
A new pointset was created! If we avoid this, everything works fine:
This is probably explained by:
so, when p gets pickled, then 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:
Incidentally, this also explains:
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:
|
comment:5
Replying to @nbruin:
Indeed. I am now experimenting with making
|
comment:6
Replying to @pjbruin:
Isn't But as you know, it can introduce difficult to debug memory leaks in some cases... |
comment:7
Replying to @simon-king-jena:
It seems to work quite well, except it took me some time to figure out the following caveat:
Apparently it makes a difference whether the default |
comment:8
Replying to @pjbruin:
Certainly not here. Anyway, the problem is that caching happens in the The point is that 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: Problem: Suppose you have a hierarchy of classes. They all inherit the One may think that this could be possible to do, for example, in Perhaps this is worth a different ticket, perhaps not (because of the bad ration of complication and gain). |
Commit: |
Author: Peter Bruin |
comment:10
So what Simon is recommending (I believe) is to add a method: @staticmethod
def __classcall__(cls, R, S=None):
# Do any other parsing/standardization that's needed here
# In particular, if S is None, then set it to ZZ following
# the __init__ of Scheme
return super(AffineScheme, cls).__classcall__(R, S) or perhaps call it However I disagree with this change: --- a/src/sage/categories/homset.py
+++ b/src/sage/categories/homset.py
@@ -505,16 +505,12 @@ class Homset(Set_generic):
sage: loads(H.dumps()) is H
True
- Homsets of non-unique parents are non-unique as well::
+ Homsets of unique parents are unique as well::
sage: H = End(AffineSpace(2, names='x,y'))
sage: loads(dumps(AffineSpace(2, names='x,y'))) is AffineSpace(2, names='x,y')
- False
- sage: loads(dumps(AffineSpace(2, names='x,y'))) == AffineSpace(2, names='x,y')
True
sage: loads(dumps(H)) is H
- False
- sage: loads(dumps(H)) == H
True
""" I think it would be much better to instead change it to a non-unique parent. EDIT (clarification) - By 'it' above I meant the parent in the test. |
comment:11
Replying to @tscrim:
That's the way to work around the limitation of However, what I was mentioning was an approach to leverage the limitation generally, without the need to implement |
comment:12
Replying to @tscrim:
Homsets are cached by identity 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. |
comment:13
I'd expect that caching strategies need amending if schemes move over to 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 I haven't checked, but the fact that point sets are cached on the scheme seems likely to instantiate a scenario as above.
You'd better have good reasons for condemning data structures to |
comment:14
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).
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:16
Replying to @tscrim:
I thought about this, but I think it does not make sense, because
Done in previous commit. |
comment:17
Replying to @nbruin:
You are right, and this is a reason why people are forced to use forking for long computations...
In this case it is probably because an elliptic curve is also a projective curve. From def __init__(self, K, ainvs):
self.__base_ring = K
self.__ainvs = tuple(K(a) for a in ainvs)
if self.discriminant() == 0:
raise ArithmeticError("invariants " + str(ainvs) + " define a singular curve")
PP = projective_space.ProjectiveSpace(2, K, names='xyz');
x, y, z = PP.coordinate_ring().gens()
a1, a2, a3, a4, a6 = ainvs
f = y**2*z + (a1*x + a3*z)*y*z \
- (x**3 + a2*x**2*z + a4*x*z**2 + a6*z**3)
plane_curve.ProjectiveCurve_generic.__init__(self, PP, f) |
comment:18
Replying to @pjbruin:
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
Thanks! Replying to @nbruin:
I thought Related question: Do we have a (documented) mechanism for freeing the |
comment:19
Replying to @tscrim:
They are: as long as the object lives, we remember it in a 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. Another issue is that libsingular polynomial rings are, unfortunately, immortal. See #13447. 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.
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:
However, sage behaviour after this is likely to be questionable (so, basically no worse than before). |
comment:20
Replying to @nbruin:
Ahh, I see what the problem is. One of these days, this will have been explained to me enough times I'll remember it.
So then would you expect #13447 to help with the leak here? Is it worthwhile to try and finish #13447? 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?
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. |
comment:21
Replying to @tscrim:
Just to clarify, #13447 is orthogonal to this ticket; the leak occurs both with and without the attached branch. I checked that the leak goes away after applying --- a/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
+++ b/src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
@@ -245,7 +245,6 @@ from sage.misc.sage_eval import sage_eval
import sage.libs.pari.gen
import polynomial_element
-permstore=[]
cdef class MPolynomialRing_libsingular(MPolynomialRing_generic):
def __cinit__(self):
@@ -367,8 +366,6 @@ cdef class MPolynomialRing_libsingular(MPolynomialRing_generic):
from sage.rings.polynomial.polynomial_element import PolynomialBaseringInjection
base_inject = PolynomialBaseringInjection(base_ring, self)
self.register_coercion(base_inject)
- #permanently store a reference to this ring until deallocation works reliably
- permstore.append(self)
def __dealloc__(self):
r""" which should be done as part of #13447. My branch does not introduce any additional leak as far as I can see.
It seems to me that #13447 and this ticket can and should be fixed independently of each other, and there is currently no separate problem requiring another ticket. |
comment:22
Replying to @pjbruin:
That doesn't prove that properly solving #13447 would leave the memory leak in place.
I do agree with that. 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). |
Reviewer: Volker Braun |
Changed branch from u/pbruin/17008-AffineScheme_unique to |
The following used to work in Sage, but does not anymore.
Define
Then
and
Note that this depends crucially on the
@fork
, 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.Since this definitely worked a few months ago, my gut says that it might have something to do with the changes introduced in #11474 and I've taken the liberty to include some of you who worked on this. I hope that's okay for you.
CC: @simon-king-jena @JohnCremona @pjbruin
Component: algebraic geometry
Author: Peter Bruin
Branch/Commit:
daaaee8
Reviewer: Volker Braun
Issue created by migration from https://trac.sagemath.org/ticket/17008
The text was updated successfully, but these errors were encountered: