Opened 6 years ago
Closed 6 years ago
#17533 closed enhancement (fixed)
Clean up parent() and related functions
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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 6 years ago by
- Description modified (diff)
comment:2 Changed 6 years ago by
- Description modified (diff)
comment:3 Changed 6 years ago by
- Description modified (diff)
comment:4 Changed 6 years ago by
- Description modified (diff)
comment:5 Changed 6 years ago by
- Summary changed from Fix parent() strangeness to Clean up parent() and related functions
comment:6 Changed 6 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 6 years ago by
- Commit set to 29ce9a14eeb40f3019913c73e0f110896fb35228
- Status changed from new to needs_review
comment:8 Changed 6 years ago by
(being curious)
comment:9 follow-up: ↓ 10 Changed 6 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 6 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 6 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 6 years ago by
- Status changed from needs_info to needs_review
comment:13 Changed 6 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 6 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 6 years ago by
- Commit changed from 977b5977269261b14ace008187aece45e589fe7c to 2303c2e9c0de9863d1fbc99ecf7fdf1c388fd03a
comment:16 follow-up: ↓ 18 Changed 6 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 6 years ago by
- Status changed from needs_work to needs_review
comment:18 in reply to: ↑ 16 ; follow-up: ↓ 19 Changed 6 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 6 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 6 years ago by
Conflicts, probably with #10779
comment:21 Changed 6 years ago by
- Status changed from positive_review to needs_work
comment:22 Changed 6 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 6 years ago by
- Status changed from needs_work to needs_review
comment:24 Changed 6 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 6 years ago by
- Status changed from needs_review to positive_review
comment:26 Changed 6 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