Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sage.matrix: Resolve circular imports without using __init__.py #30784

Closed
mkoeppe opened this issue Oct 17, 2020 · 13 comments
Closed

sage.matrix: Resolve circular imports without using __init__.py #30784

mkoeppe opened this issue Oct 17, 2020 · 13 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 17, 2020

This is preparation for making sage.matrix a namespace package in #28925.

CC: @simon-king-jena @tscrim @tobiasdiez @videlec @kiwifb

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: ea0d15a

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/30784

@mkoeppe mkoeppe added this to the sage-9.3 milestone Oct 17, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

New commits:

ea0d15asage.matrix: Resolve circular imports without using __init__.py

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

Commit: ea0d15a

@tobiasdiez
Copy link
Contributor

comment:3

What's the advantage of using global variables? I think python is also caching the imported module under the hood (https://docs.python.org/3/reference/import.html#the-module-cache).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 17, 2020

comment:4

I'm imitating existing code in Cython modules here, for example in src/sage/matrix/matrix_double_dense.pyx or src/sage/rings/complex_mpc.pyx. I have not timed it, but I would guess it has smaller overhead than the import mechanism.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 1, 2020

comment:6

Green patchbot, needs review. (One other patchbot fails with an unrelated error.)

@kiwifb
Copy link
Member

kiwifb commented Nov 1, 2020

comment:7

LGTM

@kiwifb
Copy link
Member

kiwifb commented Nov 1, 2020

Reviewer: François Bissey

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 1, 2020

comment:8

Thanks!

@tobiasdiez
Copy link
Contributor

comment:9

Without double-checking I would guess that the python import cache is heavily optimized. But even given the minimal performance penalty, I would suggest to using a simple import in favor of the global variable. The latter is a possible source of bugs (devs forgetting to use import before using it) and heavily complicates the work of static type checking tools. But maybe I'm also missing some other advantages of this approach (@francois).

@vbraun
Copy link
Member

vbraun commented Nov 22, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants