Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#5756 closed enhancement (fixed)

[with patch; positive review] improve coverage of rings/morphism.pyx (and fix 5 bugs in morphism.pyx)

Reported by: William Stein Owned by: Michael Abshoff
Priority: major Milestone: sage-3.4.1
Component: doctest coverage Keywords:
Cc: Robert Bradshaw Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by William Stein)

Fix the coverage of morphism.pyx, which is bad.

Attachments (1)

trac_5756.patch (20.0 KB) - added by William Stein 14 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 14 years ago by William Stein

Description: modified (diff)

comment:2 Changed 14 years ago by William Stein

Description: modified (diff)

BUGS FOUND:

  1. bug in cmp
    10:56 < wstein-5756> Wow, I was reading the __cmp__ for ring lifting maps.
    10:56 < wstein-5756> Check out this bug:
    10:56 < wstein-5756> Zmod(8).lift() == Zmod(10).lift()
    10:56 < wstein-5756> True
    10:56 < wstein-5756> Any two lifting maps are always equal.
    10:56 < wstein-5756> Ouch.
    
  1. Another bug related to cmp: #5758 (weird "hello")
  1. nonzero is wrong for ring morphisms, since Sage does have the 0 ring where 0 == 1, so this code was wrong:
        def __nonzero__(self):
            return True
    
  1. Calling .lift() on a morphism returns None. This is a bug that was caused by cythonizing morphism.pyx:
    sage: R.<x,y> = QQ[]; R.hom([x,x]).lift() is None
    True
    

comment:3 Changed 14 years ago by William Stein

Summary: improve coverage of rings/morphism.pyx[with patch; needs review] improve coverage of rings/morphism.pyx (and fix 5 bugs in morphism.pyx)

BUGS FOUND:

  1. im_gens() returns a mutable list, which makes it trivial to *break* a morphism after it is created:
    sage: R.<x,y> = QQ[]
    sage: f = R.hom([x,x+y])
    sage: f(x)
    x
    sage: f.im_gens()[0] = 5
    sage: f.im_gens()
    [5, x + y]
    sage: f(x)
    5
    

Changed 14 years ago by William Stein

Attachment: trac_5756.patch added

comment:4 Changed 14 years ago by John Cremona

Starting to review this, which is in itself non-trivial!

There is some strange terminology here. I'm not sure what a "Set-theoretic ring endomorphism of Integer Ring" is meant to be, let alone a "set-theoretic ring". I think that what is meant is (in the first case) a map between rings which is not a ring homomorphism, such as a section of a surjective map.

Also the term "lift" is used for such a section, i.e. if f:R-->S is the surjective ring hom and h:S-->R is a section (so f(h(s))==s for all s in S) then the map h is being called a lift, where I would say that the element h(s) is a lift of s. And "cover"? Here R is being called a cover of S?

I think it would be helpful if somewhere in this file this terminology is defined since not all of it is so standard...

A more proper review will follow.

comment:5 Changed 14 years ago by Michael Abshoff

Cc: Robert Bradshaw added

Robert: In case you are reviewing this - all doctest in my rc3 merge tree with this patch applied pass.

Cheers,

Michael

comment:6 Changed 14 years ago by Robert Bradshaw

Summary: [with patch; needs review] improve coverage of rings/morphism.pyx (and fix 5 bugs in morphism.pyx)[with patch; positive review] improve coverage of rings/morphism.pyx (and fix 5 bugs in morphism.pyx)

I agree that the notation could be made more consistent, but this patch simply documents what is there (which is good) and fixes some bugs.

One thing I noticed, which is not just common to this patch, is that when we return a list that we don't want people to change (e.g. im_gens) we (hopefully) make a copy. This is why tuples were invited, shouldn't we just be using those instead?

comment:7 in reply to:  6 Changed 14 years ago by Michael Abshoff

Replying to robertwb:

I agree that the notation could be made more consistent, but this patch simply documents what is there (which is good) and fixes some bugs.

Yeah, getting it in should be the main goal here and now.

One thing I noticed, which is not just common to this patch, is that when we return a list that we don't want people to change (e.g. im_gens) we (hopefully) make a copy. This is why tuples were invited, shouldn't we just be using those instead?

implemented instead of invited I assume?

Either way, can you please open a followup ticket so that this doesn't get lost.

Cheers,

Michael

comment:8 Changed 14 years ago by Michael Abshoff

Resolution: fixed
Status: newclosed

Merged in Sage 3.4.1.rc3.

Cheers,

Michael

comment:9 Changed 14 years ago by Robert Bradshaw

I meant invented...

Followup at #5802, perhaps there should be a followup to John Cremona's comments as well.

Note: See TracTickets for help on using tickets.