#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: |
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 9 years ago by
- Keywords days38 added
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 follow-up: ↓ 4 Changed 9 years ago by
- 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
comment:4 in reply to: ↑ 3 Changed 9 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 9 years ago by
- Status changed from needs_work to needs_review
comment:6 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:7 Changed 9 years ago by
- Work issues commit message deleted
comment:8 Changed 9 years ago by
Thanks!
comment:9 Changed 9 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 9 years ago by
- Cc mguaypaq added
comment:11 Changed 9 years ago by
- 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
comment:12 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:13 follow-up: ↓ 14 Changed 9 years ago by
- Status changed from needs_review to positive_review
comment:14 in reply to: ↑ 13 Changed 9 years ago by
comment:15 Changed 9 years ago by
- Merged in set to sage-5.1.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
Typo is in
Parent.discover_coerce_map_from
and not inParent.discover_action
as your commit message says.