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: 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 jdemeyer)

  1. Keep only inplace_threshold in coerce.pxi (that can be dealt with in another ticket) and move everything else to more appropriate places.
  2. Define the global parent() function in structure/element.pyx and document it.
  3. Move have_same_parent() to structure/element.pyx.
  4. Remove superfluous parent() methods (where parent() is defined exactly as in the base Element class, so by inheritance it's not needed).

Change History (26)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 5 years ago by jdemeyer

  • Summary changed from Fix parent() strangeness to Clean up parent() and related functions

comment:6 Changed 5 years ago by jdemeyer

  • 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 jdemeyer

  • Commit set to 29ce9a14eeb40f3019913c73e0f110896fb35228
  • Status changed from new to needs_review

New commits:

18b968dtrac #10962: make Set_PythonType pickable
97f3984trac #10962: fix documentation
da81792trac #10962: fix unpickle (Sequences is not a parent)
4b5c373Remove useless C/API calls; improve error message
f28f02aDefine parent() to be type() for SageObject
29ce9a1Clean up coerce.pxi

comment:8 Changed 5 years ago by ncohen

(being curious)

comment:9 follow-up: Changed 5 years ago by vdelecroix

  • 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 jdemeyer

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 git

  • Commit changed from 29ce9a14eeb40f3019913c73e0f110896fb35228 to a6eebf8f538d33c880791283cfa1ed2930664cc7

Branch pushed to git repo; I updated commit sha1. New commits:

a6eebf8Minor fixes to parent()

comment:12 Changed 5 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:13 Changed 5 years ago by vdelecroix

  • 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:

977b597remove trailing whitespaces

comment:14 Changed 5 years ago by jdemeyer

  • 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 jdemeyer

  • Commit changed from 977b5977269261b14ace008187aece45e589fe7c to 2303c2e9c0de9863d1fbc99ecf7fdf1c388fd03a

Avoid a merge conflict with #17654.


New commits:

2303c2eMove lazy import to avoid merge conflict

comment:16 follow-up: Changed 5 years ago by vdelecroix

  • 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 vdelecroix

  • Status changed from needs_work to needs_review

comment:18 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by 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.

comment:19 in reply to: ↑ 18 Changed 5 years ago by vdelecroix

  • 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 vbraun

Conflicts, probably with #10779

comment:21 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:22 Changed 5 years ago by git

  • 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
db023d4Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779
2e30025trac #10779 corrected 3 doctests
9d72c81Merge branch 'u/chapoton/10779' of ssh://trac.sagemath.org:22/sage into 10779
9531937trac #10779 correct failing doctest + more doc changes
54396cdMerge branch 'u/chapoton/10779' into 6.4
dbf722eRemove gcd, lcm, xgcd from element.pyx
549795cMerge ticket/10779 into ticket/17533

comment:23 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:24 Changed 5 years ago by git

  • Commit changed from 549795cda6bd76ce218431dac1176a8923866027 to 730dc597b6b86d8f37b39b1beaa17ee67a8e6cae

Branch pushed to git repo; I updated commit sha1. New commits:

730dc59Merge tag '6.5.beta6' into ticket/17533

comment:25 Changed 5 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:26 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/17533 to 730dc597b6b86d8f37b39b1beaa17ee67a8e6cae
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.