Opened 6 years ago

Closed 4 years ago

#21880 closed defect (worksforme)

MorphismMethods are ignored in tab completion

Reported by: saraedum Owned by:
Priority: trivial Milestone: sage-duplicate/invalid/wontfix
Component: categories Keywords: tab completion
Cc: Merged in:
Authors: Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description

#21879 implements is_injective on the level of a category's MorphismMethods. However, these are currently not shown in tab completion:

sage: f = QQ.hom(QQ)
sage: f.is_inj<TAB> # offers no completion

Change History (12)

comment:1 Changed 6 years ago by saraedum

Branch: u/saraedum/morphismmethods_are_ignored_in_tab_completion

comment:2 Changed 6 years ago by saraedum

Commit: 47351161e911c46793ee5eb5562f414b9de776c1
Status: newneeds_review

New commits:

4735116Lookup names in `_abstract_element_class` not just the category's `element_class.

comment:3 Changed 6 years ago by saraedum

I removed the not tested comment from another doctest because it actually already worked.

comment:4 Changed 6 years ago by nbruin

from .misc import dir_with_other_class

importing does add a measurable penalty, even if the binding is already present. So you should not do that in code that might end up in a critical loop (such as method lookup)

def f(N):
    m=0
    for n in xrange(N):
        from sage.structure.misc import dir_with_other_class
        m+=1
    return m

def g(N):
    m=0
    for n in xrange(N):
        m+=1
    return m

sage: %timeit f(1000)
1000 loops, best of 3: 1.38 ms per loop
sage: %timeit g(1000)
10000 loops, best of 3: 127 µs per loop

There are already imports from sage.structure.misc in that file, so I think you can just import the method on the module level.

comment:5 Changed 6 years ago by saraedum

Is that really an issue? If I understand your test case correctly, this costs about 1µs. dir is so slow that this does probably not make a difference:

sage: %timeit dir(1)
1000 loops, best of 3: 608 µs per loop

If I am not mistaken, __dir__ is not used in attribute lookup. At least https://docs.python.org/3/reference/datamodel.html says that __dir__ is "Called when dir() is called on the object. A sequence must be returned. dir() converts the returned sequence to a list and sorts it." and dir() says that "dir() is supplied primarily as a convenience for use at an interactive prompt [...]".

comment:6 in reply to:  5 Changed 6 years ago by nbruin

Replying to saraedum:

Is that really an issue? If I understand your test case correctly, this costs about 1µs. dir is so slow that this does probably not make a difference:

sage: %timeit dir(1)
1000 loops, best of 3: 608 µs per loop

If I am not mistaken, __dir__ is not used in attribute lookup. At least https://docs.python.org/3/reference/datamodel.html says that __dir__ is "Called when dir() is called on the object. A sequence must be returned. dir() converts the returned sequence to a list and sorts it." and dir() says that "dir() is supplied primarily as a convenience for use at an interactive prompt [...]".

Possibly. I later saw you inherited the local import. As a general rule, you shouldn't use local imports unless there is some undesirable effect from doing it at start-up. In this case there's not (you can just add the function to an existing from ... import ...), so you'd be improving the code in general by moving the import to the top of the file.

comment:7 Changed 6 years ago by saraedum

I don't really agree. Global imports tend to create cyclic dependencies (though not in this case of course) and make the code harder to refactor. The performance hit is usually irrelevant. I find code easier to read if you can see immediately where a symbol comes from. I guess this is a matter of style but I am not sure that this is really "a general rule".

comment:8 Changed 6 years ago by saraedum

Though the majority on google is on your side…

comment:9 Changed 5 years ago by saraedum

Status: needs_reviewneeds_work
Work issues: merge conflicts

comment:10 Changed 5 years ago by tscrim

Authors: Julian Rüth
Branch: u/saraedum/morphismmethods_are_ignored_in_tab_completion
Commit: 47351161e911c46793ee5eb5562f414b9de776c1
Milestone: sage-7.5sage-duplicate/invalid/wontfix
Reviewers: Travis Scrimshaw
Status: needs_workneeds_review
Work issues: merge conflicts

This works for me on 8.1.beta2.

comment:11 Changed 5 years ago by saraedum

Status: needs_reviewpositive_review

comment:12 Changed 4 years ago by embray

Resolution: worksforme
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.