Opened 8 months ago
Closed 7 months ago
#22268 closed defect (fixed)
copy for schememorphism_points not deep enough
Reported by:  bhutz  Owned by:  

Priority:  minor  Milestone:  sage7.6 
Component:  algebraic geometry  Keywords:  
Cc:  rlmiller  Merged in:  
Authors:  Ben Hutz  Reviewers:  Johannes Huisman, Rebecca Lauren Miller 
Report Upstream:  N/A  Work issues:  
Branch:  9f5ab31 (Commits)  Commit:  9f5ab317dc4040e4fe1d5065972f09a942943fd6 
Dependencies:  Stopgaps: 
Description
The copy function for scheme points does not actually produce a copy of the coords. So if you change a value of the coordinates it changes both the point and the copy.
sage: P.<x,y>=ProjectiveSpace(ZZ,1) sage: Q=P(152,113) sage: copy(Q) is Q False sage: copy(Q)._coords is Q._coords True
The last output should also be false.
Change History (22)
comment:1 Changed 8 months ago by
 Branch set to u/bhutz/t/22268/copy_error
 Cc rlmiller added
 Commit set to 582718a844fe5e441b8a25b6410782abc098a469
 Status changed from new to needs_review
comment:2 Changed 8 months ago by
 Summary changed from copy for schememropisms_points not deep enough to copy for schememorphism_points not deep enough
comment:3 Changed 8 months ago by
 Reviewers set to Johannes Huisman
 Status changed from needs_review to positive_review
comment:4 Changed 8 months ago by
 Reviewers changed from Johannes Huisman to Johannes Huisman, Rebecca Lauren Miller
Looks good to me.
comment:5 Changed 8 months ago by
 Status changed from positive_review to needs_work
Merge conflict...
comment:6 Changed 8 months ago by
 Commit changed from 582718a844fe5e441b8a25b6410782abc098a469 to 76aa6813b544356a0132fc2a1958e1ba7ebc65ef
Branch pushed to git repo; I updated commit sha1. New commits:
76aa681  Merge branch 7.6.beta2 into t/22268/t/22268/copy_error

comment:7 Changed 8 months ago by
Merged.
However, I'm getting a test failure in projective_morphism that is unrelated to this change. It seems that this fails for me in 7.6.beta2 also.
File "src/sage/schemes/projective/projective_morphism.py", line 853, in sage.schemes.projective.projective_morphism.SchemeMorphism_polynomial_projective_space.dynatomic_polynomial Failed example: f.dynatomic_polynomial(2) Expected: 2.00000000000000*x^2 + 0.999999999999999*y^2 Got: 0.666666666666667*x^2 + 0.333333333333333*y^2
Looking at the doctest, both the 'Expected' value and the 'Got' value are actually wrong. There is already a ticket open for fixing some other dynatomic polynomial issues, so I'll add this failure there.
comment:8 Changed 8 months ago by
What is happening in that example is that the quo_rem fails to return a nonzero reminder, so then maxima is tried. The division in maxima (line 964) is not checked to see if the remainder is zero (which it is not).
This example is unable to be exactly divided, so it should return the rational function.
comment:9 Changed 8 months ago by
oops, meant to add that to 22265.
comment:10 Changed 8 months ago by
This is not really an issue. Points are hashable, so copy(Q) is Q
wouldn't even be a problem. Once you start changing coordinates, you're already in undefined territory (it's an "_" attribute for a reason ...):
sage: P.<x,y>=ProjectiveSpace(ZZ,1) sage: Q=P(1,2) sage: hash(Q) 6819944855328768534 sage: Q._coords[1]=0 sage: hash(Q) 3713081631936575706
Hashable means immutable; certainly immutable as far as the hash value is concerned.
comment:11 followup: ↓ 12 Changed 8 months ago by
I understand your point in theory, but I think in practice this is not the case.
As user, if I create a point P in projective space. Then make a copy Q=copy(P). Then scale the coordinates of P by some value P.scale_by(2) or P.normalize_coordinates(). I do not expect the coordinates of Q to also change.
comment:12 in reply to: ↑ 11 Changed 8 months ago by
Replying to bhutz:
As user, if I create a point P in projective space. Then make a copy Q=copy(P). Then scale the coordinates of P by some value P.scale_by(2) or P.normalize_coordinates(). I do not expect the coordinates of Q to also change.
As a user I would not even expect the coordinates of P to change, since P advertises having a welldefined hash.
However, I see that scale_by
and normalize_coordinates
do not change the equality or hash of the point. So those are sortof OK.
If you just change Q._coords
to a tuple, you'd be done (after fixing the resulting failures). A tuple should be more efficient here anyway.
comment:13 Changed 8 months ago by
Yes, the hash was made to be independent of the choice of representation. Although I do see that does not quite work correctly as written, if the representation is not in standard form over a field, it isn't normalized first. But that is easily fixed.
changing the ._coords to a tuple does fix the copy 'issue' and scale_by and normalize_coordinates can be made to work with that. I'm fine with making this ticket that take approach instead.
The paradigm shift of having scale_by and normalize_coordinates return a new point instead of changing the coordinates in place is one I do not prefer. If this is in conflict with how most other parts of Sage works, then an argument could be made to make this conform, but it would a major change in how all scheme points/maps work.
comment:14 Changed 8 months ago by
Just to be clear. my argument for the current behavior is that those functions are not changing the point, merely the current representation of the coordinates of that point.
comment:15 Changed 8 months ago by
 Commit changed from 76aa6813b544356a0132fc2a1958e1ba7ebc65ef to a40a742483e5e81978541e2f0f9f5f4a6da9cd32
comment:16 Changed 8 months ago by
Well, here is what it would look like changing the ._coords for points to a tuple and ._polys for maps to a tuple.
This works for everything in /schemes/. I'm currently running the tests on all of sage to see if there are any other places that need updated with this change.
New commits:
b6973a7  22268: use tuple for point coords and map polys

a40a742  Merge branch 7.6.beta2 into 22268

comment:17 Changed 8 months ago by
 Commit changed from a40a742483e5e81978541e2f0f9f5f4a6da9cd32 to b2ee7cd96be91c116b089891e00367fa3cd8cf8f
Branch pushed to git repo; I updated commit sha1. New commits:
b2ee7cd  22268: another doctest fix

comment:18 Changed 8 months ago by
 Commit changed from b2ee7cd96be91c116b089891e00367fa3cd8cf8f to 9f5ab317dc4040e4fe1d5065972f09a942943fd6
Branch pushed to git repo; I updated commit sha1. New commits:
9f5ab31  22268: general cleanup

comment:19 Changed 8 months ago by
 Status changed from needs_work to needs_review
There was another failure on a long test and I did some general cleanup.
Here is a version where points/maps are switched to tuples and the couple associated corrections needed for that to work.
comment:20 Changed 7 months ago by
Is the current version, switching to tuples, preferred to the first commit which just adds an additional layer of copy for the list?
comment:21 Changed 7 months ago by
 Status changed from needs_review to positive_review
Now that we are using tuples:
sage: P.<x,y>=ProjectiveSpace(ZZ,1) sage: Q=P(152,113) sage: copy(Q) is Q False sage: copy(Q)._coords is Q._coords True
This last output is correct.
comment:22 Changed 7 months ago by
 Branch changed from u/bhutz/t/22268/copy_error to 9f5ab317dc4040e4fe1d5065972f09a942943fd6
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
22268: fix copy for scheme points