Opened 11 months ago

Last modified 6 days ago

#23807 needs_review defect

different affine patches are the same object in memory

Reported by: bhutz Owned by:
Priority: major Milestone: sage-8.4
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: u/raghukul01/affine_patch_23807 (Commits) Commit: 820be598eae1c1556a89bf473f1f9719465fd9d5
Dependencies: Stopgaps:

Description (last modified by nbruin)

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 (42)

comment:1 Changed 11 months ago by bhutz

  • Authors set to paulfili, atowsley
  • Branch set to u/bhutz/affine_patch_23807
  • Commit set to f143fb8c49a9d5ecc19d858fec501e3452ebfae6
  • Status changed from new to needs_review

New commits:

f143fb823807: make affine patches distinct objects

comment:2 Changed 11 months ago by roed

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 11 months ago by nbruin

  • 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 to AffineSpace (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 on AffineSpace
  • 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 into UniqueRepresentation.

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.

Last edited 11 months ago by nbruin (previous) (diff)

comment:4 follow-up: Changed 11 months ago by bhutz

  • Authors changed from paulfili, atowsley to Ben Hutz
  • 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 11 months ago by nbruin

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 follow-up: Changed 11 months ago by bhutz

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 11 months ago by nbruin

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 11 months ago by bhutz

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(x-y)
sage: Y = B.subscheme(x-y)
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(x-y)
sage: Y = P2.subscheme(x-y)
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 11 months ago by nbruin

  • 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 all-the-way 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.

Last edited 11 months ago by nbruin (previous) (diff)

comment:10 Changed 11 months ago by nbruin

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 follow-up: Changed 11 months ago by 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).

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 11 months ago by nbruin

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 follow-up: Changed 11 months ago by 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).

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 ; follow-up: Changed 11 months ago by 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. 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 non-caching, 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.

Last edited 11 months ago by nbruin (previous) (diff)

comment:15 in reply to: ↑ 14 ; follow-up: Changed 11 months ago by 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.

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.

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 11 months ago by nbruin

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 non-trivial 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.

Last edited 11 months ago by nbruin (previous) (diff)

comment:17 Changed 7 weeks ago by raghukul01

  • 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 sage-devel in parallel to trac for the discussion.

comment:18 Changed 7 weeks ago by raghukul01

  • Cc ​tscrim added; tcscrims removed

comment:19 follow-up: Changed 7 weeks ago by 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?

comment:20 in reply to: ↑ 19 Changed 6 weeks ago by pbruin

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 6 weeks ago by pbruin

As a more constructive contribution, I made a branch u/pbruin/23807-proposal to sketch the kind of solution that I have in mind.

comment:22 Changed 6 weeks ago by bhutz

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 3 weeks ago by raghukul01

  • Branch changed from u/bhutz/affine_patch_23807 to u/raghukul01/affine_patch_23807

comment:24 Changed 3 weeks ago by tscrim

  • Commit changed from f143fb8c49a9d5ecc19d858fec501e3452ebfae6 to 812722b1db6ad408a8250a8f91a3596df39da070
  • Keywords sagedays@icerm added
  • Milestone changed from sage-8.1 to sage-8.4

New commits:

88f82caUpgrade cysignals to 1.7.2
885f1f9adjust for minor system-dependent floating point variation on this test; #25815
208364aUpdated SageMath version to 8.3.rc2
c6cfd94Trac 23807: proposal for making projective embedding part of defining data
b08f802Trac 23807: get projective embedding of affine subscheme from ambient affine scheme
812722b23807: Some modifications

comment:25 Changed 3 weeks ago by git

  • Commit changed from 812722b1db6ad408a8250a8f91a3596df39da070 to 91862d6adfb1ab81f24ace6543b5b62ee6b0e0a8

Branch pushed to git repo; I updated commit sha1. New commits:

91862d623807: UniqueRepresentation added for ProjectiveSpaces

comment:26 Changed 3 weeks ago by git

  • Commit changed from 91862d6adfb1ab81f24ace6543b5b62ee6b0e0a8 to 5e706587f55a6de48e68f088bad7952f92fe70bf

Branch pushed to git repo; I updated commit sha1. New commits:

5e7065823807: Modified generator names for affine_patch

comment:27 Changed 3 weeks ago by raghukul01

  • Keywords gsoc2018 added
  • Status changed from needs_info to needs_review

Apart from documentation updates this branch is ready for testing.

comment:28 Changed 3 weeks ago by tscrim

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.

Last edited 3 weeks ago by tscrim (previous) (diff)

comment:29 Changed 3 weeks ago by tscrim

  • Status changed from needs_review to needs_work

comment:30 Changed 3 weeks ago by bhutz

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):
    
  • 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 follow-up: Changed 2 weeks ago by raghukul01

  • 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 weeks ago by tscrim

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 weeks ago by git

  • Commit changed from 5e706587f55a6de48e68f088bad7952f92fe70bf to 34ed3e3c9da29ce0de0a837c43da29eb277a6620

Branch pushed to git repo; I updated commit sha1. New commits:

da9465fMerge branch 'u/raghukul01/affine_patch_23807' of git://trac.sagemath.org/sage into affine_patch_23807
b94b7b723807: Corrected some doctests
da804ceMerge branch 'affine_patch_23807' into affine_patch_23807_rc3
34ed3e323807: Corrected dehomogenize in projective dynamics

comment:34 Changed 2 weeks ago by raghukul01

  • Status changed from needs_work to needs_review

Corrected doctests

comment:35 Changed 2 weeks ago by raghukul01

  • Authors changed from Ben Hutz to Ben Hutz, Peter Bruin, Raghukul Raman
  • Reviewers set to Ben Hutz, Travis Scrimshaw

comment:36 Changed 11 days ago by bhutz

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 10 days ago by tscrim

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 follow-up: Changed 10 days ago by tscrim

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 monkey-patching too). This is because the two extra parameters default to None and do not change. Now you will get unequal (because they are non-identical) 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 while-we-are-at-it, can you change the doc of specialization in generic/morphism.py to be properly formatted (by de-indenting the ::)?

Your __classcall__ is missing doctests (create two objects with not-identical 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 code-locality 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 ; follow-up: Changed 7 days ago by 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,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 7 days ago by git

  • Commit changed from 34ed3e3c9da29ce0de0a837c43da29eb277a6620 to 820be598eae1c1556a89bf473f1f9719465fd9d5

Branch pushed to git repo; I updated commit sha1. New commits:

cf69a4dMerge branch 'u/raghukul01/affine_patch_23807' of git://trac.sagemath.org/sage into affine_patch_23807
820be5923807: Added some changes

comment:41 in reply to: ↑ 39 Changed 6 days ago by tscrim

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,in Affine_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 6 days ago by bhutz

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.

Note: See TracTickets for help on using tickets.