Opened 4 years ago
Closed 3 years ago
#19552 closed enhancement (fixed)
images and preimages for projective subscheme
Reported by:  bhutz  Owned by:  bhutz 

Priority:  minor  Milestone:  sage6.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 )
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 4 years ago by
 Branch set to u/bhutz/ticket/19552
comment:2 Changed 4 years ago by
 Commit set to 8a0c020329824d932d1f3bb846455fad5547d1eb
comment:3 Changed 4 years ago by
 Commit changed from 8a0c020329824d932d1f3bb846455fad5547d1eb to 0aac0fe06164bfb15e4bf792c3e90b219e1a119c
Branch pushed to git repo; I updated commit sha1. New commits:
0aac0fe  19552: removed leftover code

comment:4 Changed 4 years ago by
 Description modified (diff)
 Status changed from new to needs_review
This is ready for another set of eyes.
comment:5 Changed 4 years ago by
 Commit changed from 0aac0fe06164bfb15e4bf792c3e90b219e1a119c to 5635becc9dd31f1e81c10f5422b37607e3ecdaa3
Branch pushed to git repo; I updated commit sha1. New commits:
5635bec  19552: fix issue with symbolic base rings

comment:6 Changed 4 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_info
Hello,
 Could you split the documentation strings into a one line description followed by a paragraph.
 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)
.
 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 usingf(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 followup: ↓ 8 Changed 4 years ago by
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 nthiterate 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 4 years ago by
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 withnth_iterate
existing, I can make this private.
On the other hand, nth_iterate
is very hard to found with tabcompletion. 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 bisided 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, usingorbit
for both would make sense, but I don't think it does in general.
This should be definitely disallowed in general!
comment:9 Changed 4 years ago by
 Commit changed from 5635becc9dd31f1e81c10f5422b37607e3ecdaa3 to 45697b745f19b2387ed28afd7e787b84a4a238cb
Branch pushed to git repo; I updated commit sha1. New commits:
45697b7  19552: adjust docstrings, rename function

comment:10 Changed 4 years ago by
 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 4 years ago by
 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 sagedevel, 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)
toif not isinstance(N, (list,tuple))
. Similarly, changeif normalize == True
withif 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 aTypeError
.
 Why are you using a copy of
self
inorbit
?
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 bythis 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)
withQ = 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 wrotereturn the subscheme
why is this unique? why it does exist? The following looks fishysage: PS.<x,y,z> = ProjectiveSpace(ZZ, 2) sage: H = End(PS) sage: f = H([x^2, x^2, x^2]) sage: X = PS.subscheme([xy]) 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 4 years ago by
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 4 years ago by
 Commit changed from 45697b745f19b2387ed28afd7e787b84a4a238cb to df4701bddec0408e5074078888f8a1f784745c6c
Branch pushed to git repo; I updated commit sha1. New commits:
df4701b  19552: fix issues from review

comment:14 Changed 4 years ago by
 Commit changed from df4701bddec0408e5074078888f8a1f784745c6c to ab4c42daa999e63000fe824f173a0eaa90defef8
Branch pushed to git repo; I updated commit sha1. New commits:
ab4c42d  19552: fixes for orbit and nth_iterate

comment:15 Changed 4 years ago by
 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 4 years ago by
 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 sagedevel.
comment:17 Changed 4 years ago by
 Commit changed from ab4c42daa999e63000fe824f173a0eaa90defef8 to 1ee5379e0ead79803de8100488d6c2549cea364a
Branch pushed to git repo; I updated commit sha1. New commits:
1ee5379  19552: remove duplicated code

comment:18 Changed 4 years ago by
 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 4 years ago by
 Commit changed from 1ee5379e0ead79803de8100488d6c2549cea364a to 1d65e7ae78c3c29b428c9155bd89130c53f5c313
Branch pushed to git repo; I updated commit sha1. New commits:
1d65e7a  19552: add check parameter to forward and preimage

comment:20 Changed 4 years ago by
added a check
parameter as the is_morphism()
check can be very slow, especially for symbolic maps.
comment:21 Changed 4 years ago by
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 3 years ago by
 Commit changed from 1d65e7ae78c3c29b428c9155bd89130c53f5c313 to 373ecb8a933903a2625a95f90fc1ea36253aac1e
Branch pushed to git repo; I updated commit sha1. New commits:
373ecb8  19552: minor correction

comment:23 Changed 3 years ago by
It seems that the code for forward_image is implemented for any morphism P^{n} > P^{m }(or n1, m1 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 followup: ↓ 25 Changed 3 years ago by
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 3 years ago by
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?)
comment:26 Changed 3 years ago by
 Commit changed from 373ecb8a933903a2625a95f90fc1ea36253aac1e to 01073fd96ead38a53fe117da06578a97892a55fb
Branch pushed to git repo; I updated commit sha1. New commits:
01073fd  19552: adding differing domain and codomain examples

comment:27 Changed 3 years ago by
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 3 years ago by
 Reviewers changed from Vincent Delecroix to Vincent Delecroix, Solomon Vishkautsan
comment:29 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:30 Changed 3 years ago by
 Branch changed from u/bhutz/ticket/19552 to 01073fd96ead38a53fe117da06578a97892a55fb
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
19552: rearranged code placement, added checks