Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#12919 closed defect (fixed)

Typo in Parent.discover_coerce_map_from

Reported by: Nicolas M. Thiéry Owned by: Robert Bradshaw
Priority: major Milestone: sage-5.1
Component: coercion Keywords: days38
Cc: Sage Combinat CC user, Mathieu Guay-Paquet Merged in: sage-5.1.beta1
Authors: Nicolas M. Thiéry, Mathieu Guay-Paquet Reviewers: André Apitzsch
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Nicolas M. Thiéry)

This patch fixes an obvious typo in discover_action. Alas, it's in a seldom used branch, and I could not extract a suitable regression test (I stumbled on the issue with a large coercion graph using quite some experimental code). And even then, the regression would not necessarily robustly catch the broken branch.

If someone wants to play further, here is the kind of thing I (unsuccessfully) tried:

class P(Parent):
    def __init__(self):
        Parent.__init__(self, category=Sets())
    Element=ElementWrapper
A = P(); a = A("a")
B = P(); b = A("b")
C = P(); c = A("c")
D = P(); d = A("d")
Hom(A,B)(lambda x: b).register_as_coercion()
Hom(B,C)(lambda x: c).register_as_coercion()
Hom(C,D)(lambda x: d).register_as_coercion()
Hom(D,A)(lambda x: a).register_as_coercion()

But I guess this patch is obvious enough to could go as is

Attachments (1)

trac_12919_coercion_typo_parent-nt.patch (2.0 KB) - added by Nicolas M. Thiéry 11 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 11 years ago by Nicolas M. Thiéry

Keywords: days38 added
Status: newneeds_review

comment:2 Changed 11 years ago by Nicolas M. Thiéry

Description: modified (diff)

comment:3 Changed 11 years ago by aapitzsch

Reviewers: André Apitzsch
Status: needs_reviewneeds_work
Summary: Typo in Parent.discover_actionTypo in Parent.discover_coerce_map_from
Work issues: commit message

Typo is in Parent.discover_coerce_map_from and not in Parent.discover_action as your commit message says.

comment:4 in reply to:  3 Changed 11 years ago by Nicolas M. Thiéry

Replying to aapitzsch:

Typo is in Parent.discover_coerce_map_from and not in Parent.discover_action as your commit message says.

Good catch, thanks :-)

comment:5 Changed 11 years ago by Nicolas M. Thiéry

Status: needs_workneeds_review

comment:6 Changed 11 years ago by Robert Bradshaw

Status: needs_reviewpositive_review

comment:7 Changed 11 years ago by aapitzsch

Work issues: commit message

comment:8 Changed 11 years ago by Nicolas M. Thiéry

Thanks!

comment:9 Changed 11 years ago by Mathieu Guay-Paquet

Here is a minimal example which illustrates the problem:

sage: class P(Parent):
....:     def __init__(self):
....:         Parent.__init__(self, category=Sets())
....:     Element=ElementWrapper
....:     
sage: A = P(); a = A('a')
sage: B = P(); b = B('b')
sage: C = P(); c = C('c')
sage: D = P(); d = D('d')
sage: Hom(A, B)(lambda x: b).register_as_coercion()
sage: Hom(B, A)(lambda x: a).register_as_coercion()
sage: Hom(C, B)(lambda x: b).register_as_coercion()
sage: Hom(D, C)(lambda x: c).register_as_coercion()
sage: A(d)
---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)

/home/mguaypaq/<ipython console> in <module>()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__call__ (sage/structure/parent.c:7906)()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.convert_map_from (sage/structure/parent.c:15248)()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_convert_map_from (sage/structure/parent.c:15399)()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14021)()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_coerce_map_from (sage/structure/parent.c:14958)()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.coerce_map_from (sage/structure/parent.c:14021)()

/opt/sage-5.0.rc0/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.discover_coerce_map_from (sage/structure/parent.c:14966)()

UnboundLocalError: local variable 'connecting' referenced before assignment

With the patch, the output is now (as it should be):

sage: class P(Parent):
....:     def __init__(self):
....:         Parent.__init__(self, category=Sets())
....:     Element=ElementWrapper
....:     
sage: A = P(); a = A('a')
sage: B = P(); b = B('b')
sage: C = P(); c = C('c')
sage: D = P(); d = D('d')
sage: Hom(A, B)(lambda x: b).register_as_coercion()
sage: Hom(B, A)(lambda x: a).register_as_coercion()
sage: Hom(C, B)(lambda x: b).register_as_coercion()
sage: Hom(D, C)(lambda x: c).register_as_coercion()
sage: A(d)
'a'

This could be added as a doctest to detect the problem, but maybe it's not necessary for such a clear typo. I would also note that this comment from #7420 points out a cleaner way to fix the problem, but this patch is fine too.

comment:10 Changed 11 years ago by Mathieu Guay-Paquet

Cc: Mathieu Guay-Paquet added

comment:11 Changed 11 years ago by Nicolas M. Thiéry

Authors: Nicolas M. ThiéryNicolas M. Thiéry, Mathieu Guay-Paquet
Status: positive_reviewneeds_work

Excellent, thanks for the doctest! I confirm it catches the issue on my machine. I'll upload shortly an updated patch, and set the ticket back to needs review. Please double check, and set back to positive review.

Changed 11 years ago by Nicolas M. Thiéry

comment:12 Changed 11 years ago by Nicolas M. Thiéry

Status: needs_workneeds_review

comment:13 Changed 11 years ago by aapitzsch

Status: needs_reviewpositive_review

comment:14 in reply to:  13 Changed 11 years ago by Nicolas M. Thiéry

Replying to aapitzsch:

positive review

Thanks!

comment:15 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.1.beta1
Resolution: fixed
Status: positive_reviewclosed

comment:16 Changed 11 years ago by Keshav Kini

It seems #12990 is a duplicate of this. Should we keep the doctest on the patch at #12990, and add it to the doctest you guys added? Or just scrap that ticket?

Note: See TracTickets for help on using tickets.