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: sage-7.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 bhutz

  • Authors set to Ben Hutz
  • Branch set to u/bhutz/t/22268/copy_error
  • Cc rlmiller added
  • Commit set to 582718a844fe5e441b8a25b6410782abc098a469
  • Status changed from new to needs_review

New commits:

582718a22268: fix copy for scheme points

comment:2 Changed 8 months ago by bhutz

  • 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 johanneshuisman

  • Reviewers set to Johannes Huisman
  • Status changed from needs_review to positive_review

comment:4 Changed 8 months ago by rlmiller

  • Reviewers changed from Johannes Huisman to Johannes Huisman, Rebecca Lauren Miller

Looks good to me.

comment:5 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict...

comment:6 Changed 8 months ago by git

  • Commit changed from 582718a844fe5e441b8a25b6410782abc098a469 to 76aa6813b544356a0132fc2a1958e1ba7ebc65ef

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

76aa681Merge branch 7.6.beta2 into t/22268/t/22268/copy_error

comment:7 Changed 8 months ago by bhutz

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 bhutz

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 bhutz

oops, meant to add that to 22265.

comment:10 Changed 8 months ago by nbruin

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

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 nbruin

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 well-defined hash.

However, I see that scale_by and normalize_coordinates do not change the equality or hash of the point. So those are sort-of 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.

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

comment:13 Changed 8 months ago by bhutz

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 bhutz

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 git

  • Commit changed from 76aa6813b544356a0132fc2a1958e1ba7ebc65ef to a40a742483e5e81978541e2f0f9f5f4a6da9cd32

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

b6973a722268: use tuple for point coords and map polys
a40a742Merge branch 7.6.beta2 into 22268

comment:16 Changed 8 months ago by bhutz

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:

b6973a722268: use tuple for point coords and map polys
a40a742Merge branch 7.6.beta2 into 22268

comment:17 Changed 8 months ago by git

  • Commit changed from a40a742483e5e81978541e2f0f9f5f4a6da9cd32 to b2ee7cd96be91c116b089891e00367fa3cd8cf8f

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

b2ee7cd22268: another doctest fix

comment:18 Changed 8 months ago by git

  • Commit changed from b2ee7cd96be91c116b089891e00367fa3cd8cf8f to 9f5ab317dc4040e4fe1d5065972f09a942943fd6

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

9f5ab3122268: general clean-up

comment:19 Changed 8 months ago by bhutz

  • Status changed from needs_work to needs_review

There was another failure on a long test and I did some general clean-up.

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 bhutz

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 rlmiller

  • 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 vbraun

  • Branch changed from u/bhutz/t/22268/copy_error to 9f5ab317dc4040e4fe1d5065972f09a942943fd6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.