Opened 7 years ago

Closed 7 years ago

#17727 closed enhancement (fixed)

Remove redundant parent_c() functions

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.5
Component: coercion Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 3a2e193 (Commits, GitHub, GitLab) Commit: 3a2e193c06f8b26ff18c9555460209a62e54f3db
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

In Sage, we have parent_c defined in 4 places, remove all except for sage/structure/element.pxd

We copy a short-cut from sage/symbolic/function.pxd for numbers not inheriting from Element (in particular Python int and float): we assume that these do not have a parent.

Change History (12)

comment:1 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/17727
  • Created changed from 02/04/15 09:36:33 to 02/04/15 09:36:33
  • Modified changed from 02/04/15 11:12:16 to 02/04/15 11:12:16

comment:4 Changed 7 years ago by jdemeyer

  • Commit set to 3547ea8990d2c2f76001df02dac213956c515e07
  • Status changed from new to needs_review

New commits:

3547ea8Remove redundant parent_c() functions

comment:5 in reply to: ↑ description Changed 7 years ago by mmezzarobba

Replying to jdemeyer:

We copy a short-cut from sage/symbolic/function.pxd for numbers not inheriting from Element (in particular Python int and float): we assume that these do not have a parent.

IMHO it would be nice to add a note to that effect in the docstring of s.s.element.parent. (Not that I really think anyone assigns parents to objects derived from built-in number types... but the concept is important enough that subtle points in its definition should be documented.)

comment:6 follow-up: Changed 7 years ago by vdelecroix

According to Python documentation

int PyNumber_Check(PyObject *o)
    Returns 1 if the object o provides numeric protocols,
    and false otherwise. This function always succeeds.

So even sage integers, floating points numbers and many others will answer true to that (I checked). And from the specification I do not see how such test can be fast. I do not have anything against using PyNumber_Check but the associated comment is definitely not appropriate.

Related question: why should we check for a method .parent()? Would the following be invalid?

cdef parent_c(x):
    if isintance(x, Element):
        return (<Element> x)._parent
    else:
        return type(x)

Vincent

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by mmezzarobba

Replying to vdelecroix:

According to Python documentation

int PyNumber_Check(PyObject *o)
    Returns 1 if the object o provides numeric protocols,
    and false otherwise. This function always succeeds.

So even sage integers, floating points numbers and many others will answer true to that (I checked). And from the specification I do not see how such test can be fast.

I don't understand your comment. As far as I understand PyNumber_Check should amount to check if the corresponding PyTypeObject's tp_as_number is non-NULL, and that should indeed be pretty fast.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by vdelecroix

Replying to mmezzarobba:

Replying to vdelecroix:

According to Python documentation

int PyNumber_Check(PyObject *o)
    Returns 1 if the object o provides numeric protocols,
    and false otherwise. This function always succeeds.

So even sage integers, floating points numbers and many others will answer true to that (I checked). And from the specification I do not see how such test can be fast.

I don't understand your comment. As far as I understand PyNumber_Check should amount to check if the corresponding PyTypeObject's tp_as_number is non-NULL, and that should indeed be pretty fast.

You are definitely right

int
PyNumber_Check(PyObject *o)
{
    return o && o->ob_type->tp_as_number &&
           (o->ob_type->tp_as_number->nb_int ||
        o->ob_type->tp_as_number->nb_float);
}

... (but it answers True on much more instances than only int/float) ...

Most important is the other question: why should we return x.parent() if it is not an element?

Vincent

comment:9 in reply to: ↑ 8 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Most important is the other question: why should we return x.parent() if it is not an element?

Because some classes which really should be an Element aren't actually an Element. See #17578 for example. If all such bugs are fixed, we could remove that additional test.

comment:10 Changed 7 years ago by git

  • Commit changed from 3547ea8990d2c2f76001df02dac213956c515e07 to 3a2e193c06f8b26ff18c9555460209a62e54f3db

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

3a2e193Improve documentation

comment:11 Changed 7 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:12 Changed 7 years ago by vbraun

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