Opened 4 years ago

Closed 4 years ago

#16834 closed defect (fixed)

Change ring fails for affine morphisms.

Reported by: gjorgenson Owned by: gjorgenson
Priority: minor Milestone: sage-6.4
Component: algebraic geometry Keywords:
Cc: bhutz Merged in:
Authors: Grayson Jorgenson Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 4b84926 (Commits) Commit: 4b8492632a6064f3396c7a49815b1bffa161ded0
Dependencies: Stopgaps:

Description (last modified by gjorgenson)

When calling the change ring function on affine morphisms an index out of bounds error occurs:

 sage: A.<x,y,z> = AffineSpace(RR,3)
 sage: h = Hom(A,A)
 sage: f = h([x^2+1.5,y^3,z^5-2.0])
 sage: f.change_ring(CC)

Change History (17)

comment:1 Changed 4 years ago by gjorgenson

  • Branch set to u/gjorgenson/ticket/16834
  • Created changed from 08/15/14 18:01:08 to 08/15/14 18:01:08
  • Modified changed from 08/15/14 18:01:08 to 08/15/14 18:01:08

comment:2 Changed 4 years ago by gjorgenson

  • Commit set to d3075c649bf1d23c07bed646775f7ed84fe4d7eb
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

d3075c6Minor change in change_ring function for scheme morphisms.

comment:3 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

I'd like to see the example a little more complex. Say at least dimension 2 with non-constant polynomials.

For the code itself you can get rid of the helper list F with the following

G=[F.change_ring(R) for F in self._polys]

Another thought. What happens when the domain and codomain are different? This code is assuming that the domain and codomain are the same.

comment:4 Changed 4 years ago by bhutz

  • Reviewers set to Ben Hutz

comment:5 Changed 4 years ago by git

  • Commit changed from d3075c649bf1d23c07bed646775f7ed84fe4d7eb to 0a29b1b9788b97fa7bbd20d9443afe7118be673a

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

0a29b1bMinor changes in morphism.py to import list and change_ring for scheme polynomial morphisms

comment:6 Changed 4 years ago by gjorgenson

  • Status changed from needs_work to needs_review

comment:7 Changed 4 years ago by gjorgenson

  • Description modified (diff)

comment:8 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

I think the endomorphism check needs some work, since we really want is instead of ==. Here is a suggestion to simplify things:

T=self.domain().change_ring(R)
if self.is_endomorphism():
    H=End(T)
else:
    H=Hom(T,self.codomain().change_ring(R))

comment:9 Changed 4 years ago by git

  • Commit changed from 0a29b1b9788b97fa7bbd20d9443afe7118be673a to ecc938e4adf473ee0cae75e67d279bbe18a120f7

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

ecc938eMinor change in change ring function for polynomial scheme morphisms

comment:10 Changed 4 years ago by gjorgenson

  • Status changed from needs_work to needs_review

Is the use of is_endomorphism() more sound than using == for the comparison of the domain and codomain?

Last edited 4 years ago by gjorgenson (previous) (diff)

comment:11 Changed 4 years ago by bhutz

I think it is faster, since you don't have to access and change_ring the codomain unless you really need it and it makes the code easily human readable.

The functionality looks fine at this point, but while you're fixing this function, there are a couple other things to fix

1) there are many instances of trailing white space that need to be removed

2) fix the formatting for the earlier examples for this function

3) the output doesn't seem like an accurate description. Perhaps something more like in the description: a :class:SchemeMorphism_polynomial which is self coerced to R

comment:12 Changed 4 years ago by git

  • Commit changed from ecc938e4adf473ee0cae75e67d279bbe18a120f7 to ef35bccf56617d62ec88d9c94f738984a7788893

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

ef35bccMinor changes to change ring function for polynomial scheme morphisms

comment:13 Changed 4 years ago by bhutz

  • Branch changed from u/gjorgenson/ticket/16834 to u/bhutz/ticket/16834
  • Modified changed from 08/20/14 16:53:51 to 08/20/14 16:53:51

comment:14 Changed 4 years ago by bhutz

  • Commit changed from ef35bccf56617d62ec88d9c94f738984a7788893 to 4b8492632a6064f3396c7a49815b1bffa161ded0
  • Status changed from needs_review to positive_review

Just removed some extra/trailing white space. This looks good.


New commits:

4b84926removed extra whitespace

comment:15 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Author name

comment:16 Changed 4 years ago by bhutz

  • Authors set to Grayson Jorgenson
  • Status changed from needs_work to positive_review

comment:17 Changed 4 years ago by vbraun

  • Branch changed from u/bhutz/ticket/16834 to 4b8492632a6064f3396c7a49815b1bffa161ded0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.