Opened 3 years ago
Closed 19 months ago
#23807 closed defect (fixed)
different affine patches are the same object in memory
Reported by:  bhutz  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  algebraic geometry  Keywords:  sagedays@icerm, gsoc2018 
Cc:  paulfili, atowsley, pbruin, tscrim  Merged in:  
Authors:  Ben Hutz, Peter Bruin, Raghukul Raman  Reviewers:  Ben Hutz, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e67687d (Commits)  Commit:  e67687d5ddfa5a3338209859981b915d6545494a 
Dependencies:  Stopgaps: 
Description (last modified by )
Currently
sage: PP = ProjectiveSpace(QQ,1) sage: AA = PP.affine_patch(0) sage: BB = PP.affine_patch(1) sage: AA is BB True
These patches are isomorphic, but should not be the same object.
Finally, since this ticket is discovering modification of global immutable objects, I think it deserves status "major" at the very least.
Change History (53)
comment:1 Changed 3 years ago by
 Branch set to u/bhutz/affine_patch_23807
 Commit set to f143fb8c49a9d5ecc19d858fec501e3452ebfae6
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
I'm not sure that your solution is the best fix for the underlying problem. This discrepency seems strange:
sage: AffineSpace(QQ,2) is AffineSpace(QQ,2) True sage: ProjectiveSpace(QQ,2) is ProjectiveSpace(QQ,2) False
Maybe AffineSpace
shouldn't be unique, for the same reasons ProjectiveSpace
isn't?
comment:3 Changed 3 years ago by
 Description modified (diff)
 Priority changed from minor to major
 Status changed from needs_review to needs_work
Changing AffineScheme
(yes, it's that high up) to UniqueRepresentation
happened on:
https://trac.sagemath.org/ticket/17008
It was already predicted we'd be having unintended consequences from this and indeed here we are.
If we're keeping UniqueRepresentation?, then we must make AfffineSpace? immutable. In affine_patch
we have this line:
AA._default_embedding_index = i
(where AA is either a freshly constructed affine space or an affine space that was passed in). We can't make that change, so
 AA as an optional parameter to
affine_patch
must be removed _default_embedding_index
must be a construction parameter toAffineSpace
(if the default is "None" and the projective closure of the space is asked, a projective space should be created and the right affine patch should be forced into the projective space  clearly projective spaces are caching their affine patches, not the other way around) If affine space is going to be
UniqueRep
then it should probably cache projective space rather than the other way around. We already have a problem there now:sage: P2.<x,y,z>=ProjectiveSpace(QQ,2); sage: A=P2.affine_patch(2) sage: A.projective_embedding() == P2 False
(the variable names get lost)
 I would recommend using something along the lines of
names=[repr(a) for a in self.gens()] names.pop(i) AA = AffineSpace(n, self.base_ring(), names = names, default_embedding_index = i)
to get generator names. Given that we cannot make assumptions on what the variable names of the projective space are, I don't think we can do much else than just dropping the variable name whose hyperplane we're removing. If you do want to have a modification, perhaps something like
[repr(a)+"_aff" for a in self.gens()]
or so.
EDIT: I think we're forced to choose between the following:
 Ditch
UniqueRepresentation
onAffineSpace
 Ditch
default_embedding
entirely and accept that one cannot get back projective closures from affine patches (in fact that's already not really possible anyway).  make
ProjectiveSpace
also intoUniqueRepresentation
.
The reason is that with unique rep affine spaces, we're always going to have an issue with
P2a.<x,y,z>=ProjectiveSpace(QQ,2) Aa=P2a.affine_patch(0) P2b.<x,y,z>=ProjectiveSpace(QQ,2) Ab=P2b.affine_patch(0)
With UniqueRepresentation
, we're going to have that Aa is identical to Ab, because there's no info to distinguish the two, so there's no room for Aa
and Ab
to lead back to P2a
and P2b
respectively.
comment:4 followup: ↓ 5 Changed 3 years ago by
 Cc paulfili atowsley added
One correction as it affects choice (2): .projective_embedding is just the map, you are trying to compare to the codomain, and that works.
sage: P2.<x,y,z>=ProjectiveSpace(QQ,2); sage: A=P2.affine_patch(2) sage: A.projective_embedding().codomain() == P2 True }}
comment:5 in reply to: ↑ 4 Changed 3 years ago by
Replying to bhutz:
One correction as it affects choice (2): .projective_embedding is just the map, you are trying to compare to the codomain, and that works.
sage: P2.<x,y,z>=ProjectiveSpace(QQ,2); sage: A=P2.affine_patch(2) sage: A.projective_embedding().codomain() == P2 True }}
Thanks! In fact:
sage: A.projective_embedding().codomain() is P2 True
That's what I originally expected to hold. It shows that A caches P2 somewhere, which makes the global modification stuff even worse.
comment:6 followup: ↓ 7 Changed 3 years ago by
The affine_patch function and the projective_embedding function update a dictionary object associated to the affine or projective space for caching of these associations. So making those spaces immutable would totally break that functionality.
I personally don't like using unique representation here, but was not involved in the changes you cited that added that. Is it even reasonable to consider undoing that?
Alternatively, it seems perhaps going unique everywhere is the next alternative. But I'm not sure how hard it would be to preserve the connections between embeddings and patches.
comment:7 in reply to: ↑ 6 Changed 3 years ago by
Replying to bhutz:
I personally don't like using unique representation here, but was not involved in the changes you cited that added that. Is it even reasonable to consider undoing that?
Alternatively, it seems perhaps going unique everywhere is the next alternative. But I'm not sure how hard it would be to preserve the connections between embeddings and patches.
I think both are reasonable solutions. I don't like UniqueRepresentation
either, so I'd have a slight preference for the former.
comment:8 Changed 3 years ago by
Here is another unfortunate current behavior
sage: A.<x,y>=AffineSpace(QQ,2) sage: B.<x,y>=AffineSpace(QQ,2) sage: A is B True sage: X = A.subscheme(xy) sage: Y = B.subscheme(xy) sage: X is Y False
Similar example, but more complicated in construction
sage: P.<x,y,z>=ProjectiveSpace(QQ,2) sage: P2.<x,y,z> = ProjectiveSpace(QQ,2) sage: X = P.subscheme(xy) sage: Y = P2.subscheme(xy) sage: X == Y, X is Y True, False sage: A = X.affine_patch(0) sage: B = Y.affine_patch(0) sage: A == B, A is B, A.ambient_space() is B.ambient_space() (True, False, True) sage: B.projective_embedding().codomain() == Y True sage: B.projective_embedding().codomain() is Y False sage: B.projective_embedding().codomain().ambient_space() is P2 True
comment:9 Changed 3 years ago by
 Cc pbruin added
Adding Peter Bruin, since the effects seen here are a direct result of his modifications on #17008. He might have an idea on how to fix this.
Options I see:
 Undo
UniqueRepresentation
on (some) schemes and deal with ensuing problems in another way
 Go alltheway on making schemes
UniqueRepresentation
, which is going to take a lot of design consideration because it means they *really* need to be immutable)
I'd expect that anything in between is going to be very hard to reason about (as it turns out: anything involving UniqueRepresentation
is hard to reason/program with in practice, because of Python's imperative streak: people get used to modifying objects)
EDIT: The real problem I see on #17008 is that consistent production of point sets relies on the fact that schemes.generic.scheme.point_homset
is cached. So, in SchemeHomset_generic.__reduce__
we need to do something along the lines of:
if <self> is the result of self.codomain().pointset(...): return PointSet,(self.codomain(),self.domain().ring_this_is_spec_of()) else: return what we did before
in order to give the cache a shot at kicking in. Once we do that, the UniqueRepresentation
from #17008 shouldn't be necessary for the problem encountered there.
Note that this applies to *all* cached routines: once you cache something, you should rewrite the pickler to allow a chance for the cache to do the lookup for you.
comment:10 Changed 3 years ago by
An argument FOR some UniqueRepresentation
: the construct AffineScheme
is really just Spec
. Every ring HAS a spec associated with it, so it would be defendable to make sure that the Spec
of a ring can be obtained from the ring reliably. This could be done by caching the thing on the ring and obtaining it from there, but since it's possible to have a ring without a spec but not its spec without the ring, the spec should probably be allowed to be GCed even when the ring is still around. A UniqueRepresentation
construction does that.
If we go this route, then obviously we need a unique polynomial ring for each affine patch for each projective space that we construct. I doubt we can key polynomial rings with projective spaces ... (that's a memory leak waiting to happen). The possibility for registering a new default embedding on a spec would need to go away.
comment:11 followup: ↓ 12 Changed 3 years ago by
Sure, I see that line of reasoning, and that is essentially what the branch on this ticket is doing (fixing the naming of the affine patches).
But if we go this route (which I'm not yet convinced of) then we'd still need to think about what things should be immutable and how that affects the connection between patches and embeddings. Not to mention fixing some of those examples involving subschemes which should also be unique by the same argument.
I'd still like to see peter's response concerning adding uniquerepresentation before making a plan.
comment:12 in reply to: ↑ 11 Changed 3 years ago by
Replying to bhutz:
Sure, I see that line of reasoning, and that is essentially what the branch on this ticket is doing (fixing the naming of the affine patches).
Yes, but see the example in comment:3. Just keying on variable names makes things slightly better, but not sufficient. Even putting the ProjectiveSpace? as part of the construction keys for the affine patches wouldn't save us because
sage: ProjectiveSpace(QQ,1)==ProjectiveSpace(QQ,1) True
UniqueRepresentation
is like a virus ...
But if we go this route (which I'm not yet convinced of) then we'd still need to think about what things should be immutable and how that affects the connection between patches and embeddings. Not to mention fixing some of those examples involving subschemes which should also be unique by the same argument.
I'm not so sure that the same argument applies: While subschemes of affine schemes may be affine again (and hence a spec of something), that's not how they are constructed in your examples. Since they are not primarily "the spec" of a ring to begin with, I don't think there's the same urgency. Obviously, spec of the coordinate ring would hopefully give the subscheme back, but I'm not sure we're supporting that, and I think we could live without that (for now).
That being said, it seems that having some schemes UR
and other not is going to be incredibly painful.
comment:13 followup: ↓ 14 Changed 3 years ago by
A useful way to store the patches/embeddings, which allows us to keep UniqueRepresentation
for affine schemes (I am strongly in favour of keeping unique representation for the reason Nils mentioned in comment;10) would be the "structure" construction that is currently used for number fields (introduced by Julian Rüth in #11670).
The idea of "structure" is the following: number fields have unique representation, but the defining data of a number field includes an optional description of how it relates to another number field. In this way, you can create a number field either on its own or (for example) as a subfield of a different number field. See sage/rings/number_field/structure.py
for details.
Analogously, we could use this construction to create, say, both an affine space on its own and a different affine space that is abstractly the same as the first one but is equipped with the extra structure of being an affine patch of a given projective space, or a closed subscheme of another scheme.
One advantage is that this would make it easier to reason about our objects, as well as to pickle them, because it is specified exactly what the defining data of a given scheme is. We do probably have to think about where/how to store the "structure" in such a way that we avoid memory leaks.
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 3 years ago by
Replying to pbruin:
A useful way to store the patches/embeddings, which allows us to keep
UniqueRepresentation
for affine schemes (I am strongly in favour of keeping unique representation for the reason Nils mentioned in comment;10) would be the "structure" construction that is currently used for number fields (introduced by Julian Rüth in #11670).
That construction is a complete memory leak. For instance:
sage: u=id(NumberField(x^2+1,name="a").absolute_field('b')) sage: import gc sage: gc.collect() 65 sage: [type(v) for v in gc.get_objects() if id(v)==u] [<class 'sage.rings.number_field.number_field.NumberField_quadratic_with_category'>]
This leaks for the following reason. If we do:
sage: K.<a>=NumberField(x^2+1) sage: L.<b>=K.absolute_field() sage: L._structure._reduction[1][0] is K True
We see that L
has a strong link to K
. So L
will now keep K
alive. What's more, via sage.rings.number_field.number_field.NumberField_version2._cache
there's now a strong global link to K
as a key in a WeakValueDictionary
with value L
, so as long as L
is alive, K
will be reachable (and not just part of a cycle).
So, if K
somehow references L
then K
and L
will not be collected. And here it is:
sage: K._cache__absolute_field.values()[0] is L True
Note that in principle it's fine for L
and K
to reference each other: the cyclic garbage collector will find those dead cycles just fine. The problem is that by using any of L
and K
as part of a key in a UniqueFactory
or similar, you're getting a strong reference to the cycle. So the key now keeps the value in the WeakValueDictionary
alive.
An important part of the problem is a double caching strategy: UniqueFactory
already provides caching. absolute_field
implements its own cache. With the introduction of structure
the first caching has been made a little better. But that now clashes with the second caching.
The situation might be slightly better if absolute_field
were noncaching, but it could well be that the required computations to arrive at the key under which the other cache operates are now too expensive. One of the reasons why UniqueRepresentation
etc. is so insidious is because it forces a caching strategy in one place, which precludes caching from being used elsewhere.
In this case a solution could be to implement absolute_field
as:
@cached_method def compute_absolute_field_data(...): ... def absolute_field(...): data = self.compute_absolute_field_data(...) L = NumberField(data,...) return L
so that the expensive data gets stored without creating or referencing the field that can be constructed with it.
comment:15 in reply to: ↑ 14 ; followup: ↓ 16 Changed 3 years ago by
Replying to nbruin:
Replying to pbruin:
A useful way to store the patches/embeddings, which allows us to keep
UniqueRepresentation
for affine schemes (I am strongly in favour of keeping unique representation for the reason Nils mentioned in comment;10) would be the "structure" construction that is currently used for number fields (introduced by Julian Rüth in #11670).That construction is a complete memory leak.
Ouch. It seems that neither the author nor the reviewers of #11670 (including me) paid attention to memory leaks.
In this case a solution could be to implement
absolute_field
as:@cached_method def compute_absolute_field_data(...): ... def absolute_field(...): data = self.compute_absolute_field_data(...) L = NumberField(data,...) return Lso that the expensive data gets stored without creating or referencing the field that can be constructed with it.
This is already how absolute_field()
is implemented; data
is the absolute defining polynomial, which is computed and cached by the method absolute_polynomial()
. Removing the @cached_method
decorator from NumberField_{generic,relative}.absolute_field()
solves the memory leak in your example:
sage: u=id(NumberField(x^2+1,name="a").absolute_field('b')) sage: import gc sage: gc.collect() 53 sage: [type(v) for v in gc.get_objects() if id(v)==u] []
comment:16 in reply to: ↑ 15 Changed 3 years ago by
Replying to pbruin:
Replying to nbruin:
Replying to pbruin:
A useful way to store the patches/embeddings, which allows us to keep
UniqueRepresentation
for affine schemes (I am strongly in favour of keeping unique representation for the reason Nils mentioned in comment;10) would be the "structure" construction that is currently used for number fields (introduced by Julian Rüth in #11670).That construction is a complete memory leak.
Ouch. It seems that neither the author nor the reviewers of #11670 (including me) paid attention to memory leaks.
Whenever you see a CachedRepresentation
or a UniqueFactory
that has included in its input parameters objects that are similar to what is returned, you can expect memory leaks (and I really mean, you should expect to be able to easily create an example as above). That is one of my main reasons to not like it. It's very difficult to use it in nontrivial examples and not create memory leaks. Basically, input to these constructors should consist of just strings and integers. If you can't do that, it's a good sign you should perhaps not use it, because someone will create a memory leak with it in the near future.
Fix suggested above now at #23851.
comment:17 Changed 2 years ago by
 Cc tcscrims added
 Status changed from needs_work to needs_info
Hi, I am working on this ticket as a part of Google Summer of Code'18. Can we start the discussion again? I needs some details to get started. I suggest using sagedevel
in parallel to trac
for the discussion.
comment:18 Changed 2 years ago by
 Cc tscrim added; tcscrims removed
comment:19 followup: ↓ 20 Changed 2 years ago by
Ideally, I'd still like to see UniqueRepresentation? removed from affine schemes. Nils sketched a possible alternative to the issue in #17008 in comment 9. Peter, does that seem like the details might be workable for the #17008 issue eliminating the need for UR for affine space?
comment:20 in reply to: ↑ 19 Changed 2 years ago by
Replying to bhutz:
Ideally, I'd still like to see UniqueRepresentation? removed from affine schemes. Nils sketched a possible alternative to the issue in #17008 in comment 9. Peter, does that seem like the details might be workable for the #17008 issue eliminating the need for UR for affine space?
I find that hard to predict; the only way to find that out is by trying. Personally I don't trust the idea of modifying mathematical objects after construction (except for caching purposes), since this is very likely to introduce bugs. An example I just found:
sage: X.<x,y> = toric_varieties.A2_Z2() sage: Y = X.subscheme([x*y, x^2]) sage: Y.is_smooth([0, 0]) True sage: Y.embedding_center() [0 : 0] sage: Y.is_smooth([0, 1]) # this test changes properties of Y! True sage: Y.embedding_center() [0 : 1]
My ideal world would be one where it is made completely explicit what the defining data of a mathematical object are, where all properties of an object can be determined from these defining data, and where these defining data are immutable. From that perspective, using UniqueRepresentation
or UniqueFactory
is a way to force oneself to be careful and consistent about these defining data, and undoing it would be a step backwards.
comment:21 Changed 2 years ago by
As a more constructive contribution, I made a branch u/pbruin/23807proposal to sketch the kind of solution that I have in mind.
comment:22 Changed 2 years ago by
Thanks Peter. I agree that it is possible to get the patching/embedding to work (maybe even without memory leaks) while keeping UR. I'm just not familiar enough with the kind of issue in #17008 to know if that fix is reasonable. From your response it sounds like for this ticket perhaps both fixes should be attempted and see what issues crop up and proceed from there.
comment:23 Changed 2 years ago by
 Branch changed from u/bhutz/affine_patch_23807 to u/raghukul01/affine_patch_23807
comment:24 Changed 2 years ago by
 Commit changed from f143fb8c49a9d5ecc19d858fec501e3452ebfae6 to 812722b1db6ad408a8250a8f91a3596df39da070
 Keywords sagedays@icerm added
 Milestone changed from sage8.1 to sage8.4
New commits:
88f82ca  Upgrade cysignals to 1.7.2

885f1f9  adjust for minor systemdependent floating point variation on this test; #25815

208364a  Updated SageMath version to 8.3.rc2

c6cfd94  Trac 23807: proposal for making projective embedding part of defining data

b08f802  Trac 23807: get projective embedding of affine subscheme from ambient affine scheme

812722b  23807: Some modifications

comment:25 Changed 2 years ago by
 Commit changed from 812722b1db6ad408a8250a8f91a3596df39da070 to 91862d6adfb1ab81f24ace6543b5b62ee6b0e0a8
Branch pushed to git repo; I updated commit sha1. New commits:
91862d6  23807: UniqueRepresentation added for ProjectiveSpaces

comment:26 Changed 2 years ago by
 Commit changed from 91862d6adfb1ab81f24ace6543b5b62ee6b0e0a8 to 5e706587f55a6de48e68f088bad7952f92fe70bf
Branch pushed to git repo; I updated commit sha1. New commits:
5e70658  23807: Modified generator names for affine_patch

comment:27 Changed 2 years ago by
 Keywords gsoc2018 added
 Status changed from needs_info to needs_review
Apart from documentation updates this branch is ready for testing.
comment:28 Changed 2 years ago by
Doctest failures
sage t long src/sage/dynamics/arithmetic_dynamics/projective_ds.py # 5 doctests failed sage t long src/sage/categories/homset.py # 2 doctests failed
See also the patchbot.
comment:29 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:30 Changed 2 years ago by
A couple other comments
 I don't understand why the parameters were added to the subsheme init. It seems like these cannot be different than the ambient space
algebraic subscheme: def __init__(self, A, polynomials, ambient_projective_space=None, default_embedding_index=None):
 Do we want to setup a ProjectiveScheme? class like the AffineScheme? class?
 The default should be immutable
P.<x,y,z>=ProjectiveSpace(QQ,2) A.<u,v> = AffineSpace(2,QQ,ambient_projective_space=P,default_embedding_index=0) A._default_embedding_index=4 A.projective_embedding()
comment:31 followup: ↓ 32 Changed 2 years ago by
 Yes exactly, why do we only have
AffineScheme
class?
 As of immutable objects, I think that issue is in most of the classes as well, consider
sage: P = ProjectiveSpace(QQ,3) sage: P.coordinate_ring() sage: P._coordinate_ring = QQ sage: P.coordinate_ring()
I dont really know how to make variables immutable in sage, there is a
set_immutable()
function for Sequences, but dont think it would work here.
 And yes, I agree we dont need to pass
ambient_projective_space=None, default_embedding_index=None
in Subscheme.
There is one issue with change in generator names, consider:
sage: P.<x,y> = ProjectiveSpace(QQ, 1) sage: f = DynamicalSystem_projective([y^2, x^2]) sage: f.dehomogenize(tuple([0,1]))
comment:32 in reply to: ↑ 31 Changed 2 years ago by
Replying to raghukul01:
 As of immutable objects, I think that issue is in most of the classes as well, consider
sage: P = ProjectiveSpace(QQ,3) sage: P.coordinate_ring() sage: P._coordinate_ring = QQ sage: P.coordinate_ring()
Most objects are semantically immutable, not syntactically (i.e., it does not raise an error when you do mutate it). The user is not suppose to access _coordinate_ring
because it starts with a leading underscore. So as far as the user is concerned, the object is immutable. So you should not be changing these things in code (which is part of the problem here).
I dont really know how to make variables immutable in sage, there is a
set_immutable()
function for Sequences, but dont think it would work here.
As mentioned above, this is basically a red herring and irrelevant.
comment:33 Changed 2 years ago by
 Commit changed from 5e706587f55a6de48e68f088bad7952f92fe70bf to 34ed3e3c9da29ce0de0a837c43da29eb277a6620
Branch pushed to git repo; I updated commit sha1. New commits:
da9465f  Merge branch 'u/raghukul01/affine_patch_23807' of git://trac.sagemath.org/sage into affine_patch_23807

b94b7b7  23807: Corrected some doctests

da804ce  Merge branch 'affine_patch_23807' into affine_patch_23807_rc3

34ed3e3  23807: Corrected dehomogenize in projective dynamics

comment:35 Changed 2 years ago by
 Reviewers set to Ben Hutz, Travis Scrimshaw
comment:36 Changed 2 years ago by
I did a look through the code as well as some notebook testing. I didn't come up with anything significant from a usage perspective. Here are a couple minor comments. Someone more familiar with the memory management issues of UR should take a look at this as well.
The doc string
If the dehomogenized morphism is an endomorphism then a :class:`DynamicalSystem_affine` given by dehomogenizing the source and target of `self` with respect to the given indices. If it is not an endomorphism then the morphism is returned.
could be simpler. It is totally dependent on the indicies, so something more like If the dehomogenizing indices are the same for the domain and codomain, then a dynamical system is returned. If the dehomogenizing indicies for the domain and codomain are different then the resulting affine patches are different and a scheme morphism is returned
Also I'd rather have a specific check than a blanket try/except
try: return F.as_dynamical_system() except: return F
AffineSpace?(n, R=None, names='x', ambient_projective_space=None,
default_embedding_index=None)
so now UR is keyed off of not just the base ring and the variable names, but also the embedding index and the embedding codomain. Does this now violate the discussion above where UR was justified by each coordinate ring giving a unique space? I can essentially now make as many different Spec(QQ[x,y]) as I'd like all different.
something is wrong here ~line 710 affine_space.py
if PP is None: PP = self._ambient_projective_space if PP is None:
comment:37 Changed 2 years ago by
Quick comment: Do not have a bare except:
statement as it can also catch things like keyboard interrupts. You should instead do except Exception:
.
comment:38 followup: ↓ 39 Changed 2 years ago by
I don't see anything outright that would cause a memory leak. Is there some infinite sequence that would be reasonable test?
I agree with Ben's comment:36 overall.
so now UR is keyed off of not just the base ring and the variable names, but also the embedding index and the embedding codomain. Does this now violate the discussion above where UR was justified by each coordinate ring giving a unique space? I can essentially now make as many different Spec(QQ[x,y]) as I'd like all different.
In principle, I don't think you can create different Specs from the construction parameters (unless you did something evil, probably involving some monkeypatching too). This is because the two extra parameters default to None
and do not change. Now you will get unequal (because they are nonidentical) affine patches if they have different embeddings/projective spaces, but from the ticket description, I am inferring they are only isomorphic, which is okay. However, I don't know the math, so I might need a little more detail about what sort of behavior you expect.
I do have some other comments on the branch:
You should not change what the test in homset.py
is actually testing. So you need to find a new parent that is not unique and construct its End
. This is something I might be able to do, but hopefully someone following this ticket knows a good example.
In affine_subscheme.py
, your added __init__
is missing doctests.
A whileweareatit, can you change the doc of specialization in generic/morphism.py
to be properly formatted (by deindenting the ::
)?
Your __classcall__
is missing doctests (create two objects with notidentical but equivalent entries and show they are the same by using is
).
This change is suspicious to me:
@@ 276,11 +276,8 @@ class AlgebraicScheme_subscheme_projective(AlgebraicScheme_subscheme): phi = AA.projective_embedding(i, PP) polys = self.defining_polynomials() xi = phi.defining_polynomials()  U = AA.subscheme([ f(xi) for f in polys ])  U._default_embedding_index = i  phi = U.projective_embedding(i, PP) + U = AA.subscheme([f(xi) for f in polys]) self.__affine_patches[i] = U  U._embedding_morphism = phi return U def _best_affine_patch(self, point):
Is the default embedding index and morphism of U
set automatically? Before it didn't seem like it did, and I am not sure you're changes make it so.
You can simplify this:
gens = [g[j] for j in range(i)] + [g[j] for j in range(i+1, n+1)] +gens = g[:i] + g[i+1:]
With passing in AA
to affine_patch
, what if AA._default_embedding_index
is not equal to i
? This feels like it will put things into an inconsistent state as that corresponding AA
is stored and used again. (Side note: this lazily construct __affine_patches = {}
makes the code way more complicated than it needs to be for no gain IMO.)
Don't you need to do some changes in affine_space.py
for projective_embedding
to take into account the _default_embedding_index
and _ambient_projective_space
?
I don't understand why you added this:
+ i = default_embedding_index + if i is not None: + i = int(i)
Such a check should be done in the AffineSpace_generic.__init__
method because that is where is it "used" (basically a codelocality reason).
Instead of having a check like this:
@@ 185,7 +190,7 @@ class AlgebraicScheme_subscheme_affine(AlgebraicScheme_subscheme): n = AA.dimension_relative() if i is None: try:  i = self._default_embedding_index + return self._embedding_morphism except AttributeError: i = int(n) else:
It would be better to set self._embedding_morphism = None
in the __init__
and then check
if self._embedding_morphism is not None: return self._embedding_morphism else: i = int(n)
It is easier to understand the program flow and makes it easier to debug (mainly, when people do typos).
comment:39 in reply to: ↑ 38 ; followup: ↓ 41 Changed 2 years ago by
Replying to tscrim:
This change is suspicious to me:
@@ 276,11 +276,8 @@ class AlgebraicScheme_subscheme_projective(AlgebraicScheme_subscheme): phi = AA.projective_embedding(i, PP) polys = self.defining_polynomials() xi = phi.defining_polynomials()  U = AA.subscheme([ f(xi) for f in polys ])  U._default_embedding_index = i  phi = U.projective_embedding(i, PP) + U = AA.subscheme([f(xi) for f in polys]) self.__affine_patches[i] = U  U._embedding_morphism = phi return U def _best_affine_patch(self, point):Is the default embedding index and morphism of
U
set automatically? Before it didn't seem like it did, and I am not sure you're changes make it so.
affine_patch function takes the embedding index as input, unlike Affine_Space
,in Affine_Subschemes
this embedding index is a necessary input. ie it cannot be None, so there is no need for _default_embedding_index
.
With passing in AA to affine_patch, what if AA._default_embedding_index is not equal to i? This feels like it will put things into an inconsistent state as that corresponding AA is stored and used again.
I don't really know how to deal with that, someone on top of the ticket suggested that AA
as a parameter should be remove, to which I agree.
(Side note: this lazily construct
__affine_patches = {}
makes the code way more complicated than it needs to be for no gain IMO.)
construction of most of these variable is done in similar way, any better method?
Don't you need to do some changes in affine_space.py for projective_embedding to take into account the _default_embedding_index and _ambient_projective_space?
I have taken then into account, any specific change, which I am missing?
comment:40 Changed 2 years ago by
 Commit changed from 34ed3e3c9da29ce0de0a837c43da29eb277a6620 to 820be598eae1c1556a89bf473f1f9719465fd9d5
comment:41 in reply to: ↑ 39 Changed 2 years ago by
Replying to raghukul01:
Replying to tscrim:
This change is suspicious to me:
@@ 276,11 +276,8 @@ class AlgebraicScheme_subscheme_projective(AlgebraicScheme_subscheme): phi = AA.projective_embedding(i, PP) polys = self.defining_polynomials() xi = phi.defining_polynomials()  U = AA.subscheme([ f(xi) for f in polys ])  U._default_embedding_index = i  phi = U.projective_embedding(i, PP) + U = AA.subscheme([f(xi) for f in polys]) self.__affine_patches[i] = U  U._embedding_morphism = phi return U def _best_affine_patch(self, point):Is the default embedding index and morphism of
U
set automatically? Before it didn't seem like it did, and I am not sure you're changes make it so.affine_patch function takes the embedding index as input, unlike
Affine_Space
,inAffine_Subschemes
this embedding index is a necessary input. ie it cannot be None, so there is no need for_default_embedding_index
.
Yes, but does the subscheme
your are constructing have the correct embedding index/morphism? I didn't necessarily see a mechanism to set these things. It is possible I missed something, but it might be a problem if not.
With passing in AA to affine_patch, what if AA._default_embedding_index is not equal to i? This feels like it will put things into an inconsistent state as that corresponding AA is stored and used again.
I don't really know how to deal with that, someone on top of the ticket suggested that
AA
as a parameter should be remove, to which I agree.
Then you probably should remove it (well, deprecate it). Another option would be to raise an error if such an AA
was going to take it into an inconsistent state.
(Side note: this lazily construct
__affine_patches = {}
makes the code way more complicated than it needs to be for no gain IMO.)construction of most of these variable is done in similar way, any better method?
Yes (IMO), initialize these during the __init__
. Actually, there is a very small speed gain once it is constructed to having it the way it is now. Although I wonder if these methods would be better served by making them @cached_method
or putting those attributes as an @lazy_attribute
.
Don't you need to do some changes in affine_space.py for projective_embedding to take into account the _default_embedding_index and _ambient_projective_space?
I have taken then into account, any specific change, which I am missing?
I don't think so.
I guess there is also a bit of a question of "Should you be able to construct an AffineSpace
without an (default) _ambient_projective_space
?"
comment:42 Changed 2 years ago by
Just commenting on two points here
 Doesn't this code in init for subscheme set the correct default index. I thought I tested this in the notebook and the subschemes were getting the correct default embeddings.
AlgebraicScheme_subscheme.__init__(self, A, polynomials) if A._ambient_projective_space is not None: self._embedding_morphism = self.projective_embedding \ (A._default_embedding_index, A._ambient_projective_space) else: self._embedding_morphism = None
 in regards to: "Should you be able to construct an AffineSpace? without an (default) _ambient_projective_space?"
If the answer here should be "no", there isn't a canonical way to choose a default embedding so you'd need to require the user to specify. The current code takes the answer as yes and has default embedding optional. From a mathematical perspective, an affine space is certainly an independent object and does not need to be attached to a projective space. Each of the possible embeddings is in some sense equally good.
To me this is very similar to the number field question. When you create a number field in Sage you have the option of specifying how it is embedded in CC (or QQBar etc). There are a number of choices for this embedding which are all in some sense equally good, and without user input, there is no "canonical" choice for this.
comment:43 Changed 23 months ago by
comment:45 Changed 20 months ago by
 Branch changed from u/raghukul01/affine_patch_23807 to u/pbruin/23807affine_patch
 Commit changed from 820be598eae1c1556a89bf473f1f9719465fd9d5 to 5d5327886dbce8f4daa8d220ac6bea3453c8cdd4
 Status changed from needs_work to needs_review
Two new commits to solve the merge conflict and the remaining TODO item.
comment:46 Changed 20 months ago by
I looked through the changes a couple days ago and it looked fine. I've been waiting for a long running computation to finish on my Sage machine so that I can run some tests. However it is still running. Not sure when it will finish, but when it does, I'll run through some testing on this ticket.
comment:47 Changed 20 months ago by
The pyflakes plugin found a number of unused imports and variables in the files touched by this ticket, see #26687.
comment:48 followup: ↓ 50 Changed 20 months ago by
 Milestone changed from sage8.4 to sage8.5
 Status changed from needs_review to needs_work
Ran through some tests today. Just one example that seems a little weird to me
sage: A.<u,v> = AffineSpace(2,QQ,default_embedding_index=1) sage: X=A.subscheme([uv]) sage: X.projective_embedding(), A._default_embedding_index (Scheme morphism: From: Closed subscheme of Affine Space of dimension 2 over Rational Field defined by: u  v To: Closed subscheme of Projective Space of dimension 2 over Rational Field defined by: x0  x1 Defn: Defined on coordinates by sending (u, v) to (u : v : 1), 1)
The subscheme is not inheriting the default embedding. So you get weird things like
sage: phi=X.projective_embedding() sage: psi=A1.projective_embedding() sage: phi(X(2,2))== psi(A1(X(2,2))) False
comment:49 Changed 19 months ago by
 Commit changed from 5d5327886dbce8f4daa8d220ac6bea3453c8cdd4 to e67687d5ddfa5a3338209859981b915d6545494a
Branch pushed to git repo; I updated commit sha1. New commits:
e67687d  Trac 23807: subschemes of affine spaces should respect projective embeddings

comment:50 in reply to: ↑ 48 Changed 19 months ago by
 Status changed from needs_work to needs_review
Replying to bhutz:
The subscheme is not inheriting the default embedding.
The new commit should fix this.
comment:51 Changed 19 months ago by
This looks good to me. Since the patch bot failed, let me run the long tests as a final check.
comment:52 Changed 19 months ago by
 Status changed from needs_review to positive_review
all tests pass.
comment:53 Changed 19 months ago by
 Branch changed from u/pbruin/23807affine_patch to e67687d5ddfa5a3338209859981b915d6545494a
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
23807: make affine patches distinct objects