Opened 19 months ago

Closed 19 months ago

Last modified 18 months ago

#20067 closed enhancement (fixed)

Change ring to QQbar fails for subschemes

Reported by: bhutz Owned by: bhutz
Priority: minor Milestone: sage-7.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(x-y)
X.change_ring(QQbar)
K.<w>=QuadraticField(2)
P.<x,y> = ProjectiveSpace(K,1)
X = P.subscheme(x-y)
H = End(X)
f = H([6*x^2+2*x*y+16*y^2,-3*x^2-4*x*y-4*y^2])
f.change_ring(QQbar)

Change History (32)

comment:1 Changed 19 months ago by bhutz

  • Branch set to u/bhutz/ticket/20067

comment:2 Changed 19 months ago by git

  • Commit set to 276ff7176dd5241a7b84e39a3db75fac4041c2e5

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

276ff7120067: improve change_ring() for subschemes

comment:3 Changed 19 months ago by bhutz

  • 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 19 months ago by git

  • Commit changed from 276ff7176dd5241a7b84e39a3db75fac4041c2e5 to 6a39223c28383e62542290d74deae6cb9a1e3cac

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

6a3922320067: try coercion first for change_ring

comment:5 Changed 19 months ago by rlmiller

  • Reviewers set to Rebecca Miller, Joseph Eisner

comment:6 Changed 19 months ago by rlmiller

  • Reviewers changed from Rebecca Miller, Joseph Eisner to Rebecca Lauren Miller, Joseph Eisner

comment:7 Changed 19 months ago by rlmiller

  1. Has coerce map from instead of try accept
  2. raise warning if embedding not specified and no coerce map

Still doing functionality testing.

comment:8 Changed 19 months ago by git

  • Commit changed from 6a39223c28383e62542290d74deae6cb9a1e3cac to 05e47bbfe1f62431520b6fbb4affb73f9802b710

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

05e47bb20067: add warning when choosing embedding

comment:9 Changed 19 months ago by bhutz

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 19 months ago by nbruin

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 well-defined), but we can still get an answer.

comment:11 follow-up: Changed 19 months ago by bhutz

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 19 months ago by nbruin

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 non-canonical guess.

comment:13 Changed 19 months ago by bhutz

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 in-line with similar other functionality, I can do that instead.

comment:14 Changed 19 months ago by git

  • Commit changed from 05e47bbfe1f62431520b6fbb4affb73f9802b710 to e28d8bec91e15ff9b99139f26c172f435dcd90f1

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

e28d8be20067: remove creation of embedding if not specified

comment:15 Changed 19 months ago by bhutz

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 19 months ago by git

  • Commit changed from e28d8bec91e15ff9b99139f26c172f435dcd90f1 to 0882474a082adffa796b90b15a964dd90a5dd50f

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

0882474Merge branch 'master' into ticket/20067

comment:17 Changed 19 months ago by bhutz

  • 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 19 months ago by git

  • Commit changed from 0882474a082adffa796b90b15a964dd90a5dd50f to 1e6769852ac9b88c50832283014615ee8ed2b6ac

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

1e6769820067: pass embeddings to change_ring

comment:19 Changed 19 months ago by git

  • Commit changed from 1e6769852ac9b88c50832283014615ee8ed2b6ac to 9001f02b60e5625ea59ce78f2220daf2b9b5f256

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

9001f0220067: minor doc fix

comment:20 Changed 19 months ago by bhutz

  • 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 19 months ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name is missing

comment:22 Changed 19 months ago by vbraun

I mean, Author name

comment:23 Changed 19 months ago by bhutz

  • Authors set to Ben Hutz
  • Status changed from needs_work to needs_review

I meant to set this to needs review. Added author.

comment:24 Changed 19 months ago by rlmiller

  • Status changed from needs_review to positive_review

Looks good to me!

comment:25 Changed 19 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, please merge in the latest beta...

comment:26 Changed 19 months ago by git

  • Commit changed from 9001f02b60e5625ea59ce78f2220daf2b9b5f256 to e73cd1fa960dad9e38430ef87b202124d2c8dd08

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

e73cd1fMerge branch '7.1.beta6' into ticket/20067

comment:27 Changed 19 months ago by bhutz

  • Status changed from needs_work to needs_review

comment:28 Changed 19 months ago by rlmiller

  • Status changed from needs_review to positive_review

Looks good to me. No error with new commits in the latest beta.

comment:29 Changed 19 months ago by vbraun

  • Branch changed from u/bhutz/ticket/20067 to e73cd1fa960dad9e38430ef87b202124d2c8dd08
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 18 months ago by jdemeyer

  • Commit e73cd1fa960dad9e38430ef87b202124d2c8dd08 deleted

What do you prefer: "Rebecca Lauren Miller" or "Rebecca Miller"?

comment:31 Changed 18 months ago by kcrisman

Given that comment:6 was done by rlmiller, I would guess the "Lauren" version.

comment:32 Changed 18 months ago by rlmiller

Yes, The Lauren Version. I will update all previous tickets.

Note: See TracTickets for help on using tickets.