Opened 5 years ago
Closed 5 years ago
#17533 closed enhancement (fixed)
Clean up parent() and related functions
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  coercion  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  730dc59 (Commits)  Commit:  730dc597b6b86d8f37b39b1beaa17ee67a8e6cae 
Dependencies:  #10962  Stopgaps: 
Description (last modified by )
 Keep only
inplace_threshold
incoerce.pxi
(that can be dealt with in another ticket) and move everything else to more appropriate places.  Define the global
parent()
function instructure/element.pyx
and document it.  Move
have_same_parent()
tostructure/element.pyx
.  Remove superfluous
parent()
methods (whereparent()
is defined exactly as in the baseElement
class, so by inheritance it's not needed).
Change History (26)
comment:1 Changed 5 years ago by
 Description modified (diff)
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Description modified (diff)
comment:4 Changed 5 years ago by
 Description modified (diff)
comment:5 Changed 5 years ago by
 Summary changed from Fix parent() strangeness to Clean up parent() and related functions
comment:6 Changed 5 years ago by
 Branch set to u/jdemeyer/ticket/17533
 Created changed from 12/20/14 23:14:15 to 12/20/14 23:14:15
 Modified changed from 12/21/14 21:54:16 to 12/21/14 21:54:16
comment:7 Changed 5 years ago by
 Commit set to 29ce9a14eeb40f3019913c73e0f110896fb35228
 Status changed from new to needs_review
comment:8 Changed 5 years ago by
(being curious)
comment:9 followup: ↓ 10 Changed 5 years ago by
 Status changed from needs_review to needs_info
Hello Jeroen,
1) Why moving "parent_c" and "have_same_parent_c"?
2) Instead of
cdef inline parent_c(x): if isinstance(x, Element): return (<Element>x)._parent elif hasattr(x, 'parent'): return x.parent() else: return type(x)
wouldn't it be faster to do
cdef inline parent_c(x): if isinstance(x, Element): return (<Element>x)._parent else: try: return x.parent() except AttributeError: return type(x)
3) In element.pyx
, the "seealso" should be a "SEEALSO".
Vincent
comment:10 in reply to: ↑ 9 Changed 5 years ago by
Replying to vdelecroix:
Hello Jeroen,
1) Why moving "parent_c" and "have_same_parent_c"?
Mainly to get rid of coerce.pxi
.
comment:11 Changed 5 years ago by
 Commit changed from 29ce9a14eeb40f3019913c73e0f110896fb35228 to a6eebf8f538d33c880791283cfa1ed2930664cc7
Branch pushed to git repo; I updated commit sha1. New commits:
a6eebf8  Minor fixes to parent()

comment:12 Changed 5 years ago by
 Status changed from needs_info to needs_review
comment:13 Changed 5 years ago by
 Branch changed from u/jdemeyer/ticket/17533 to u/vdelecroix/17533
 Commit changed from a6eebf8f538d33c880791283cfa1ed2930664cc7 to 977b5977269261b14ace008187aece45e589fe7c
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
I just removed trailing whitespaces that you introduce. Beyond that, everything looks fine.
Vincent
New commits:
977b597  remove trailing whitespaces

comment:14 Changed 5 years ago by
 Branch changed from u/vdelecroix/17533 to u/jdemeyer/ticket/17533
 Modified changed from 01/20/15 16:19:22 to 01/20/15 16:19:22
comment:15 Changed 5 years ago by
 Commit changed from 977b5977269261b14ace008187aece45e589fe7c to 2303c2e9c0de9863d1fbc99ecf7fdf1c388fd03a
comment:16 followup: ↓ 18 Changed 5 years ago by
 Status changed from positive_review to needs_work
As you set dependencies in this order, I move back this one in needs review.
comment:17 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:18 in reply to: ↑ 16 ; followup: ↓ 19 Changed 5 years ago by
Replying to vdelecroix:
As you set dependencies in this order
I didn't change any dependencies, I just added this one commit to avoid a conflict. You just have to review this last commit by itself.
comment:19 in reply to: ↑ 18 Changed 5 years ago by
 Status changed from needs_review to positive_review
Replying to jdemeyer:
Replying to vdelecroix:
As you set dependencies in this order
I didn't change any dependencies, I just added this one commit to avoid a conflict. You just have to review this last commit by itself.
Ho, I see!
comment:20 Changed 5 years ago by
Conflicts, probably with #10779
comment:21 Changed 5 years ago by
 Status changed from positive_review to needs_work
comment:22 Changed 5 years ago by
 Commit changed from 2303c2e9c0de9863d1fbc99ecf7fdf1c388fd03a to 549795cda6bd76ce218431dac1176a8923866027
Branch pushed to git repo; I updated commit sha1. New commits:
c84246e  #10779: Improve coverage test for structure/element.pyx

db023d4  Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779

2e30025  trac #10779 corrected 3 doctests

9d72c81  Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779

9531937  trac #10779 correct failing doctest + more doc changes

54396cd  Merge branch 'u/chapoton/10779' into 6.4

dbf722e  Remove gcd, lcm, xgcd from element.pyx

549795c  Merge ticket/10779 into ticket/17533

comment:23 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:24 Changed 5 years ago by
 Commit changed from 549795cda6bd76ce218431dac1176a8923866027 to 730dc597b6b86d8f37b39b1beaa17ee67a8e6cae
Branch pushed to git repo; I updated commit sha1. New commits:
730dc59  Merge tag '6.5.beta6' into ticket/17533

comment:25 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:26 Changed 5 years ago by
 Branch changed from u/jdemeyer/ticket/17533 to 730dc597b6b86d8f37b39b1beaa17ee67a8e6cae
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #10962: make Set_PythonType pickable
trac #10962: fix documentation
trac #10962: fix unpickle (Sequences is not a parent)
Remove useless C/API calls; improve error message
Define parent() to be type() for SageObject
Clean up coerce.pxi