Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#12919 closed defect (fixed)

Typo in Parent.discover_coerce_map_from

Reported by: nthiery Owned by: robertwb
Priority: major Milestone: sage-5.1
Component: coercion Keywords: days38
Cc: sage-combinat, mguaypaq 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 nthiery)

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 nthiery 9 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by nthiery

  • Keywords days38 added
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by nthiery

  • Description modified (diff)

comment:3 follow-up: Changed 9 years ago by aapitzsch

  • Reviewers set to André Apitzsch
  • Status changed from needs_review to needs_work
  • Summary changed from Typo in Parent.discover_action to Typo in Parent.discover_coerce_map_from
  • Work issues set to 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 9 years ago by nthiery

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 9 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:6 Changed 9 years ago by robertwb

  • Status changed from needs_review to positive_review

comment:7 Changed 9 years ago by aapitzsch

  • Work issues commit message deleted

comment:8 Changed 9 years ago by nthiery

Thanks!

comment:9 Changed 9 years ago by mguaypaq

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 9 years ago by mguaypaq

  • Cc mguaypaq added

comment:11 Changed 9 years ago by nthiery

  • Authors changed from Nicolas M. Thiéry to Nicolas M. Thiéry, Mathieu Guay-Paquet
  • Status changed from positive_review to needs_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 9 years ago by nthiery

comment:12 Changed 9 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 9 years ago by aapitzsch

  • Status changed from needs_review to positive_review

comment:14 in reply to: ↑ 13 Changed 9 years ago by nthiery

Replying to aapitzsch:

positive review

Thanks!

comment:15 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.1.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 Changed 9 years ago by 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.