#19552 closed enhancement (fixed)

images and preimages for projective subscheme

Reported by: bhutz Owned by: bhutz
Priority: minor Milestone: sage-6.10
Component: algebraic geometry Keywords: subscheme iteration
Cc: Merged in:
Authors: Ben Hutz Reviewers: Vincent Delecroix, Solomon Vishkautsan
Report Upstream: N/A Work issues:
Branch: 01073fd (Commits) Commit: 01073fd96ead38a53fe117da06578a97892a55fb
Dependencies: Stopgaps:

Description (last modified by bhutz)

Compute the forward and inverse images for projective subschemes under projective morphisms.

The forward image can be computed with an elimination calculation and the preimage is simply composition with the map.

This includes orbit() and nth_iterate() functions for subschemes.

Change History (30)

comment:1 Changed 23 months ago by bhutz

  • Branch set to u/bhutz/ticket/19552

comment:2 Changed 23 months ago by git

  • Commit set to 8a0c020329824d932d1f3bb846455fad5547d1eb

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

8a0c02019552: rearranged code placement, added checks

comment:3 Changed 23 months ago by git

  • Commit changed from 8a0c020329824d932d1f3bb846455fad5547d1eb to 0aac0fe06164bfb15e4bf792c3e90b219e1a119c

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

0aac0fe19552: removed leftover code

comment:4 Changed 23 months ago by bhutz

  • Authors set to Ben Hutz
  • Description modified (diff)
  • Status changed from new to needs_review

This is ready for another set of eyes.

comment:5 Changed 23 months ago by git

  • Commit changed from 0aac0fe06164bfb15e4bf792c3e90b219e1a119c to 5635becc9dd31f1e81c10f5422b37607e3ecdaa3

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

5635bec19552: fix issue with symbolic base rings

comment:6 Changed 22 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_info

Hello,

  1. Could you split the documentation strings into a one line description followed by a paragraph.
  1. For the input, I would prefer to follow the Python conventions
  • x.orbit(f, 3) -> [x, f(x), f^2(x)]
  • x.orbit(f, 2, 5) -> [f^2(x), f^3(x), f^4(x)]

In other words having a def orbit(self, f, n, m=None).

  1. Is there really a need for that many functions:
  • orbit (this is achieved by a simple loop)
  • nth_iterate (this is more or less equivalent to .orbit(f, (n,n+1))).
  • forward_image (this is more or less equivalent to .orbit(f, (0,1)))... and why not using f(self) here? If this is needed by call, then it could be a private method _forward_image.
  • preimage (this should be equivalent to .orbit(f, (-1,0)))
  • __call__

comment:7 follow-up: Changed 22 months ago by bhutz

1) I will update the docs.

2) hmm..I was matching the inputs that are used for the orbit function for affine and projective points. I was not aware of the python conventions covering such a situation. If we follow them here, then the other functions should be changed to match. This is fine by me, but I'll make the other function changes in a separate ticket.

3) orbit returns a list of points and nth_iterate returns a single point. I use the functions separately as they exists for points, so I continued the separation here. Combining them would make finding an nth-iterate slightly more cumbersome: you would have to create the list orbit(f,(n,n+1)) and then get the first element out of that list. As I didn't think there was a significant drawback with making them 2 separate functions and as a user, I'd rather have two functions.

forward_image is used by call and I debated making it private or not. I think with nth_iterate existing, I can make this private.

using orbit for preimages is a little shaky in my opinion. The forward images are single points(or varieties) but the preimages are collections of points (or varieties). Allowing something like x.orbit(-2,2) would return quite a strange object. So, I see this functionality as different. In the special case of automorphisms, using orbit for both would make sense, but I don't think it does in general.

comment:8 in reply to: ↑ 7 Changed 22 months ago by vdelecroix

Replying to bhutz:

2) hmm..I was matching the inputs that are used for the orbit function for affine and projective points. I was not aware of the python conventions covering such a situation. If we follow them here, then the other functions should be changed to match. This is fine by me, but I'll make the other function changes in a separate ticket.

I see. Then for the sake of this ticket I guess it is better to follow the convention of the other dynamical functions. The advantage of the Python way is that it is faster to parse the argument (less type checking and avoid tuple constructions) and you have the property that

orbit(n1,n2) + orbit(n2,n3) == orbit(n1,n3)

Please let it for a later ticket (if you have time).

3) orbit returns a list of points and ...

The only draw back I see is the multiplication of methods! I do not claim that any of them is useless.

forward_image is used by call and I debated making it private or not. I think with nth_iterate existing, I can make this private.

On the other hand, nth_iterate is very hard to found with tab-completion. If I would choose one name, I would go for forward_image (with an optional argument order). For word morphism (maps of the free monoid) there is an optional argument in __call__

sage: s = WordMorphism('a->ab,b->ba')
sage: s('a')
word: ab
sage: s('a', 5)
word: abbabaabbaababbabaababbaabbabaab

But of course you can not found __call__ with tab completion. But as it is a word morphism is a function it is natural to use the my_map(x) syntax instead of x.forward_image(my_map). And actually, there is also

sage: w = Word('ab')
sage: w.apply_morphism(s)
word: abba

I guess it would be good to unify the terminology here...

using orbit for preimages is a little shaky in my opinion. The forward images are single points(or varieties) but the preimages are collections of points (or varieties).

You are right, I was thiking of bijective maps for which bi-sided orbits make sense.

Allowing something like x.orbit(-2,2) would return quite a strange object. So, I see this functionality as different. In the special case of automorphisms, using orbit for both would make sense, but I don't think it does in general.

This should be definitely disallowed in general!

comment:9 Changed 22 months ago by git

  • Commit changed from 5635becc9dd31f1e81c10f5422b37607e3ecdaa3 to 45697b745f19b2387ed28afd7e787b84a4a238cb

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

45697b719552: adjust docstrings, rename function

comment:10 Changed 22 months ago by bhutz

  • Status changed from needs_info to needs_review

Docs strings adjusted and forward_image now private. I've made a note to myself to adjust how all the orbit functions take the parameters and this will be done in different ticket. As for the naming of the functions. These match all the other dynamics functionality and for this ticket I think that is most important. If it's necessary to choose more findable names, then these should be changed for all the dynamics functionality at the same time.

comment:11 Changed 22 months ago by vdelecroix

  • Status changed from needs_review to needs_work
  • In your first commit, you completely removed the file schemes/generic/morphism.py. Why is so?
  • test the error that are raised. And to fit with Python standards that was discussed on sage-devel, the error message should be lower case with no ponctuation at the end like the following
    sage: [][1]
    Traceback (most recent call last):
    ...
    IndexError: list index out of range
    
  • change if (isinstance(N,(list,tuple)) == False) to if not isinstance(N, (list,tuple)). Similarly, change if normalize == True with if normalize.
  • In
    +        except TypeError:
    +            raise TypeError("Orbit bounds must be integers")
    

or

+        except TypeError:
+            raise TypeError("Iterate number must be an integer")

there is no need to catch a TypeError to raise a TypeError.

  • Why are you using a copy of self in orbit?
  • a proejctive subscheme -> projective. Moreover the output is not a projective subscheme but a list!
  • What is this line in the documentation of orbit
    Perform the checks on subscheme initialization if ``check=True``
    
  • The mention to self in the documentation should be avoided as much as possible. You can replace by this scheme.
  • You can simplify a bit the following in nth_iterate
    +        if n == 0:
    +            return(self)
    +        else:
    +            Q = f(self)
    +            for i in range(2,n+1):
    +                Q = f(Q)
    +            return(Q)
    
    with
    Q = self
    for i in range(n):
        Q = f(Q)
    return Q
    
  • to make a list of zeros use [0] * n instead of [0 for _ in range(n)].
  • In preimage you wrote return the subscheme why is this unique? why it does exist? The following looks fishy
    sage: PS.<x,y,z> = ProjectiveSpace(ZZ, 2)
    sage: H = End(PS)
    sage: f = H([x^2, x^2, x^2])
    sage: X = PS.subscheme([x-y])
    sage: X.preimage(f)
    Closed subscheme of Projective Space of dimension 2 over Integer Ring defined by:
      0
    sage: f(X.preimage(f))
    Closed subscheme of Projective Space of dimension 2 over Integer Ring defined by:
      y - z,
      x - z
    

comment:12 Changed 22 months ago by bhutz

I don't see how morphism.py got removed. It is still there locally for me and I gave no command to stop tracking it. I'll get that back in.

yes, your example is an issue. I'll need to add a check that the map is a morphism otherwise lots of strange things can happen.

comment:13 Changed 22 months ago by git

  • Commit changed from 45697b745f19b2387ed28afd7e787b84a4a238cb to df4701bddec0408e5074078888f8a1f784745c6c

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

df4701b19552: fix issues from review

comment:14 Changed 22 months ago by git

  • Commit changed from df4701bddec0408e5074078888f8a1f784745c6c to ab4c42daa999e63000fe824f173a0eaa90defef8

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

ab4c42d19552: fixes for orbit and nth_iterate

comment:15 Changed 22 months ago by bhutz

  • Status changed from needs_work to needs_review

Corrections made. I guess I was just being overly cautious with the copy. It seems to work fine without (i.e., self remains unchanged).

I greatly increased the spectrum of failure testing as well.

Also, I was overly ambitious in my coercing in call so I scaled that back slightly. I was able to produce a weird behavior with the example in algebraic_scheme.py line 2472. That example now raises an error.

As for generic/morphism.py. I couldn't find anything locally that was causing the deletion. Looking at each commit individually (locally and on trac), nowhere was it removed. Now that I've pushed these new commits up the issue seems to have gone away. I guess trac was just confused...

comment:16 Changed 22 months ago by vdelecroix

  • Status changed from needs_review to needs_work

from copy import copy is not needed anymore in algebraic_scheme.py. There exists a very nice python tool for that

$ pyflakes src/sage/schemes/generic/algebraic_scheme.py 
src/sage/schemes/generic/algebraic_scheme.py:133: 'copy' imported but unused
src/sage/schemes/generic/algebraic_scheme.py:140: 'is_MPolynomialRing' imported but unused
src/sage/schemes/generic/algebraic_scheme.py:3238: local variable 'n' is assigned to but never used

On the other hand there is still a copy in projective_point.py.

About my comment about multiplication of methods, the code in nth_iteration is an exact copy paste of a portion of orbit...

Another oneliner simplification

dict = {}
for i in range(codom.dimension_relative()+1):
    dict.update({R.gen(i): f[i]})

can be written as

dict = {R.gen(i): f[i] for i in range(codom.dimension_relative()+1)}

I have no real competence to check the mathematical validity of the code. If you want somebody else to review that part you might ask on sage-devel.

comment:17 Changed 22 months ago by git

  • Commit changed from ab4c42daa999e63000fe824f173a0eaa90defef8 to 1ee5379e0ead79803de8100488d6c2549cea364a

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

1ee537919552: remove duplicated code

comment:18 Changed 22 months ago by bhutz

  • Status changed from needs_work to needs_review

ah yes, I see what you mean. I've removed the duplicated code and now call orbit from nth_iterate. I also noticed that kwds was not used in the algebraic scheme iterate, so I removed that as well.

comment:19 Changed 22 months ago by git

  • Commit changed from 1ee5379e0ead79803de8100488d6c2549cea364a to 1d65e7ae78c3c29b428c9155bd89130c53f5c313

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

1d65e7a19552: add check parameter to forward and preimage

comment:20 Changed 22 months ago by bhutz

added a check parameter as the is_morphism() check can be very slow, especially for symbolic maps.

comment:21 Changed 21 months ago by vdelecroix

It is better to make the keyword apparent in the function as in

def f(param1=True, param2=False):
    ....

instead of

def f(**kwds):
    param1 = kwds.get('param1', True)
    param2 = kwds.get('param2', False)
    ...

The reason is that with the former the tab completion works and you can see the list of keywords from the function signature.

There is still a

if (isinstance(N,(list,tuple))==False):

that should be replaced with

if not isinstance(N,(list,tuple)):

comment:22 Changed 21 months ago by git

  • Commit changed from 1d65e7ae78c3c29b428c9155bd89130c53f5c313 to 373ecb8a933903a2625a95f90fc1ea36253aac1e

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

373ecb819552: minor correction

comment:23 Changed 21 months ago by wishcow

It seems that the code for forward_image is implemented for any morphism Pn -> P(or n-1, m-1 to be precise with the code), yet the examples are all for endomorphisms of projective space. I think it would be better to either add examples with different domain and codomain, or add an assertion that checks n=m and states otherwise that only endomorphisms are currently implemented.

comment:24 follow-up: Changed 21 months ago by bhutz

Good point. I do think the mathematics is fine for n != m as long as the map is a morphism (which requires m >=n). Let me think some more about it and then I'll probably add some additional examples.

comment:25 in reply to: ↑ 24 Changed 21 months ago by wishcow

If I may suggest giving examples of the Veronese embedding. This is well known and people should know what their output should be. (Sorry, the Segre embedding is of cartesian product, I am guessing forward_image is not implemented for this?)

Last edited 21 months ago by wishcow (previous) (diff)

comment:26 Changed 21 months ago by git

  • Commit changed from 373ecb8a933903a2625a95f90fc1ea36253aac1e to 01073fd96ead38a53fe117da06578a97892a55fb

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

01073fd19552: adding differing domain and codomain examples

comment:27 Changed 21 months ago by bhutz

Examples added. The veronese embedding is a good idea. I also did a couple dimension 0 examples for which the output is easily verified as well.

No, we can't do the segre embedding like this. However, Segre embedding is implemented for projective products.

comment:28 Changed 21 months ago by wishcow

  • Reviewers changed from Vincent Delecroix to Vincent Delecroix, Solomon Vishkautsan

comment:29 Changed 21 months ago by wishcow

  • Status changed from needs_review to positive_review

comment:30 Changed 21 months ago by vbraun

  • Branch changed from u/bhutz/ticket/19552 to 01073fd96ead38a53fe117da06578a97892a55fb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.