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: |
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
Branch: | → u/saraedum/morphismmethods_are_ignored_in_tab_completion |
---|
comment:2 Changed 6 years ago by
Commit: | → 47351161e911c46793ee5eb5562f414b9de776c1 |
---|---|
Status: | new → needs_review |
comment:3 Changed 6 years ago by
I removed the not tested
comment from another doctest because it actually already worked.
comment:4 Changed 6 years ago by
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 follow-up: 6 Changed 6 years ago by
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 Changed 6 years ago by
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 loopIf 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." anddir()
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
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:9 Changed 5 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → merge conflicts |
comment:10 Changed 5 years ago by
Authors: | Julian Rüth |
---|---|
Branch: | u/saraedum/morphismmethods_are_ignored_in_tab_completion |
Commit: | 47351161e911c46793ee5eb5562f414b9de776c1 |
Milestone: | sage-7.5 → sage-duplicate/invalid/wontfix |
Reviewers: | → Travis Scrimshaw |
Status: | needs_work → needs_review |
Work issues: | merge conflicts |
This works for me on 8.1.beta2
.
comment:11 Changed 5 years ago by
Status: | needs_review → positive_review |
---|
comment:12 Changed 4 years ago by
Resolution: | → worksforme |
---|---|
Status: | positive_review → closed |
New commits:
Lookup names in `_abstract_element_class` not just the category's `element_class.