#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: |
Description (last modified by )
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)
Change History (17)
comment:1 Changed 11 years ago by
Keywords: | days38 added |
---|---|
Status: | new → needs_review |
comment:2 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:3 follow-up: 4 Changed 11 years ago by
Reviewers: | → André Apitzsch |
---|---|
Status: | needs_review → needs_work |
Summary: | Typo in Parent.discover_action → Typo in Parent.discover_coerce_map_from |
Work issues: | → commit message |
comment:4 Changed 11 years ago by
Replying to aapitzsch:
Typo is in
Parent.discover_coerce_map_from
and not inParent.discover_action
as your commit message says.
Good catch, thanks :-)
comment:5 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
comment:6 Changed 11 years ago by
Status: | needs_review → positive_review |
---|
comment:7 Changed 11 years ago by
Work issues: | commit message |
---|
comment:9 Changed 11 years ago by
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
Cc: | Mathieu Guay-Paquet added |
---|
comment:11 Changed 11 years ago by
Authors: | Nicolas M. Thiéry → Nicolas M. Thiéry, Mathieu Guay-Paquet |
---|---|
Status: | positive_review → 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 11 years ago by
Attachment: | trac_12919_coercion_typo_parent-nt.patch added |
---|
comment:12 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
comment:13 follow-up: 14 Changed 11 years ago by
Status: | needs_review → positive_review |
---|
comment:15 Changed 11 years ago by
Merged in: | → sage-5.1.beta1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Typo is in
Parent.discover_coerce_map_from
and not inParent.discover_action
as your commit message says.