#20067 closed enhancement (fixed)
Change ring to QQbar fails for subschemes
Reported by:  bhutz  Owned by:  bhutz 

Priority:  minor  Milestone:  sage7.1 
Component:  algebraic geometry  Keywords:  
Cc:  Merged in:  
Authors:  Ben Hutz  Reviewers:  Rebecca Lauren Miller, Joseph Eisner 
Report Upstream:  N/A  Work issues:  
Branch:  e73cd1f (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
Change ring for subschemes does not let the user specify an embedding and so fails for QQbar
K.<w>=QuadraticField(2) P.<x,y> = ProjectiveSpace(K,1) X = P.subscheme(xy) X.change_ring(QQbar)
K.<w>=QuadraticField(2) P.<x,y> = ProjectiveSpace(K,1) X = P.subscheme(xy) H = End(X) f = H([6*x^2+2*x*y+16*y^2,3*x^24*x*y4*y^2]) f.change_ring(QQbar)
Change History (32)
comment:1 Changed 2 years ago by
 Branch set to u/bhutz/ticket/20067
comment:2 Changed 2 years ago by
 Commit set to 276ff7176dd5241a7b84e39a3db75fac4041c2e5
comment:3 Changed 2 years ago by
 Status changed from new to needs_review
I added the embedding option for subschemes and made some other improvements to change_ring() in general to make it a little more robust.
comment:4 Changed 2 years ago by
 Commit changed from 276ff7176dd5241a7b84e39a3db75fac4041c2e5 to 6a39223c28383e62542290d74deae6cb9a1e3cac
Branch pushed to git repo; I updated commit sha1. New commits:
6a39223  20067: try coercion first for change_ring

comment:5 Changed 2 years ago by
 Reviewers set to Rebecca Miller, Joseph Eisner
comment:6 Changed 2 years ago by
 Reviewers changed from Rebecca Miller, Joseph Eisner to Rebecca Lauren Miller, Joseph Eisner
comment:7 Changed 2 years ago by
 Has coerce map from instead of try accept
 raise warning if embedding not specified and no coerce map
Still doing functionality testing.
comment:8 Changed 2 years ago by
 Commit changed from 6a39223c28383e62542290d74deae6cb9a1e3cac to 05e47bbfe1f62431520b6fbb4affb73f9802b710
Branch pushed to git repo; I updated commit sha1. New commits:
05e47bb  20067: add warning when choosing embedding

comment:9 Changed 2 years ago by
changing the try/except to has_coerce_map_from reduced the functionality. For example QQ to GF(3) does not have a coerce map from, but can be done.
Consequently, I only added the warning.
comment:10 Changed 2 years ago by
Can't we just make sure that base_change
or change_ring
can also be given a ring homomorphism from the base ring, to base extend to the codomain of the homomorphism? Then
X=Scheme(Q,...) k=GF(3) Xk=X.base_change(k.convert_map_from(QQ))
would work (provided we don't check if the map really is a homomorphism).
Then we can have that X.base_change(GF(3))
can still raise an error (because it's not welldefined), but we can still get an answer.
comment:11 followup: ↓ 12 Changed 2 years ago by
Not sure exactly what you are suggesting here. If it is just the ability to supply the mapping, then that is actually the main change here. There is now an optional embedding
parameter that can be specified
Xk=X_change_ring(embedding=k.convert_map_from(QQ))
The remaining issue was what to do if not specified. In this case, one is attempted to be constructed (the try/except parts...). When this is not unique, you seem to be suggesting raising an error and having the user construct the particular map they want. It seemed more user friendly to me to simply choose one and provide a warning that happened.
comment:12 in reply to: ↑ 11 Changed 2 years ago by
Replying to bhutz:
The remaining issue was what to do if not specified. In this case, one is attempted to be constructed (the try/except parts...). When this is not unique, you seem to be suggesting raising an error and having the user construct the particular map they want. It seemed more user friendly to me to simply choose one and provide a warning that happened.
The problem is that we don't have a channel to reliably report warnings through. If you print them in places where people aren't particularly looking for output, they are easily missed. In fact, STDOUT might be redirected to /dev/null, and messages on STDERR might be hard to match up with the rest of the data (if it gets read at all).
To quote the Zen of Python: "In the face of ambiguity, refuse the temptation to guess.". I'm not for following the Zen religiously, but I think it's a good principle. In this case, I think the error is a more honest and ultimately more reliable solution than making some noncanonical guess.
comment:13 Changed 2 years ago by
I understand the concern and definitely didn't want to silently guess, hence the verbose warning. However, there was some precedent since
K.<w>=CyclotomicField(3) CC(w)
just works and 'guesses' what embedding to use.
However, if just raising an error is more inline with similar other functionality, I can do that instead.
comment:14 Changed 2 years ago by
 Commit changed from 05e47bbfe1f62431520b6fbb4affb73f9802b710 to e28d8bec91e15ff9b99139f26c172f435dcd90f1
Branch pushed to git repo; I updated commit sha1. New commits:
e28d8be  20067: remove creation of embedding if not specified

comment:15 Changed 2 years ago by
on further looking, that wasn't a precedent since cyclotomic fields create an embedding by default as part of their creation.
So here is a version where I've removed the 'guessing'
comment:16 Changed 2 years ago by
 Commit changed from e28d8bec91e15ff9b99139f26c172f435dcd90f1 to 0882474a082adffa796b90b15a964dd90a5dd50f
Branch pushed to git repo; I updated commit sha1. New commits:
0882474  Merge branch 'master' into ticket/20067

comment:17 Changed 2 years ago by
 Status changed from needs_review to needs_work
I don't like the embedding keyword syntax. I'm going to check this to accept passing the ring in. This is more compatible with the other existing change_ring functions.
comment:18 Changed 2 years ago by
 Commit changed from 0882474a082adffa796b90b15a964dd90a5dd50f to 1e6769852ac9b88c50832283014615ee8ed2b6ac
Branch pushed to git repo; I updated commit sha1. New commits:
1e67698  20067: pass embeddings to change_ring

comment:19 Changed 2 years ago by
 Commit changed from 1e6769852ac9b88c50832283014615ee8ed2b6ac to 9001f02b60e5625ea59ce78f2220daf2b9b5f256
Branch pushed to git repo; I updated commit sha1. New commits:
9001f02  20067: minor doc fix

comment:20 Changed 2 years ago by
 Status changed from needs_work to positive_review
I think this new version is cleaner and is ready to be reviewed.
btw, there seems to be a doc failure on 7.1.beta5 which is merged with this ticket that is unrelated.
src/sage/matrix/matrix_double_dense.pyx
It fails with and without these changes.
comment:21 Changed 2 years ago by
 Status changed from positive_review to needs_work
Reviewer name is missing
comment:22 Changed 2 years ago by
I mean, Author name
comment:23 Changed 2 years ago by
 Status changed from needs_work to needs_review
I meant to set this to needs review. Added author.
comment:24 Changed 2 years ago by
 Status changed from needs_review to positive_review
Looks good to me!
comment:25 Changed 2 years ago by
 Status changed from positive_review to needs_work
Merge conflict, please merge in the latest beta...
comment:26 Changed 2 years ago by
 Commit changed from 9001f02b60e5625ea59ce78f2220daf2b9b5f256 to e73cd1fa960dad9e38430ef87b202124d2c8dd08
Branch pushed to git repo; I updated commit sha1. New commits:
e73cd1f  Merge branch '7.1.beta6' into ticket/20067

comment:27 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:28 Changed 2 years ago by
 Status changed from needs_review to positive_review
Looks good to me. No error with new commits in the latest beta.
comment:29 Changed 2 years ago by
 Branch changed from u/bhutz/ticket/20067 to e73cd1fa960dad9e38430ef87b202124d2c8dd08
 Resolution set to fixed
 Status changed from positive_review to closed
comment:30 Changed 2 years ago by
 Commit e73cd1fa960dad9e38430ef87b202124d2c8dd08 deleted
What do you prefer: "Rebecca Lauren Miller" or "Rebecca Miller"?
comment:31 Changed 2 years ago by
Given that comment:6 was done by rlmiller, I would guess the "Lauren" version.
comment:32 Changed 2 years ago by
Yes, The Lauren Version. I will update all previous tickets.
Branch pushed to git repo; I updated commit sha1. New commits:
20067: improve change_ring() for subschemes