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

Remove .all import of infinity #32734

Closed
mkoeppe opened this issue Oct 21, 2021 · 23 comments
Closed

Remove .all import of infinity #32734

mkoeppe opened this issue Oct 21, 2021 · 23 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 21, 2021

... throughout the Sage library.

sage.rings is becoming a namespace package in the course of modularization (#29705), so we are replacing imports from sage.rings.all throughout the Sage library.

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 97861cd

Reviewer: Kwankyu Lee

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 21, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 21, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 21, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 21, 2021

Commit: 2446ec9

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 21, 2021

New commits:

2446ec9git grep -l 'import infinity *$' | xargs sed -i.bak 's/ sage.*all import infinity *$/ sage.rings.infinity import infinity/'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2021

Changed commit from 2446ec9 to bc2d970

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2021

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

9b83d92src/sage/combinat: Remove all-import of infinity
0224b35src/sage/rings/valuation: Remove all-import of infinity
3c5750csrc/sage/modular/abvar: Remove all-import of infinity
aa797a5src/sage/rings/polynomial: Remove all-import of infinity
bb3ca7asrc/sage/modular/modform_hecketriangle: Remove all-import of infinity
8e096f4src/sage/modular/modsym: Remove all-import of infinity
58eb1f9src/sage/rings/valuation/valuation_space.py: Remove all-import of infinity
bc2d970src/sage/schemes/elliptic_curves/height.py: Remove all-import of infinity

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2021

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

c658a23src/sage/combinat/cluster_algebra_quiver/quiver_mutation_type.py: Fix typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2021

Changed commit from bc2d970 to c658a23

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2021

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

97861cdsrc/sage/rings/valuation/valuation_space.py: Fix typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 21, 2021

Changed commit from c658a23 to 97861cd

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 21, 2021

comment:6

The failure in src/sage/rings/integer.pyx is not from this ticket.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 28, 2021

comment:7

What is the new policy? Do we intend to remove all all.py eventually? If not, what should(can) remain in all.py? Or this is determined by how we split sage library? The last case is not good, I think...

Or there won't be no change in all.py but we stop importing things from it in Sage library?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 28, 2021

comment:8

All all.py are kept, for the purpose of populating the global interactive environment (sage.all).

We are just getting rid of imports from sage.PAC.KAGE.all within the Sage library. In particular, the imports from those packages sage.PAC.KAGE that become namespace packages, i.e., filled with modules from several distributions.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 28, 2021

comment:9

I am asking because you keep lines like

from sage.rings.all import AA, QQbar, CC

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 28, 2021

comment:10

Just because I can't do everything on the same ticket. These will also go away, or perhaps it is already done on another ticket.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 28, 2021

comment:11

CC, for example, is subject of #30741

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 28, 2021

comment:12

Replying to @mkoeppe:

Just because I can't do everything on the same ticket. These will also go away, or perhaps it is already done on another ticket.

Okay.

May I understand the new policy as that we never import from .all within the Sage library?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 28, 2021

comment:13

That would be a safe and easy to understand way to phrase a new policy.

But that policy would be broader than what is technically needed. For example, I have no plans to split up sage.plot into several distributions (sage.plot will remain an ordinary package with __init__.py). So it does not matter whether library code imports from sage.plot.all or sage.plot.plot.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 28, 2021

comment:14

These changes seem to increase startup time slightly. But I think we can allow this.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 28, 2021

Reviewer: Kwankyu Lee

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 28, 2021

comment:15

Thanks for reviewing!

@vbraun
Copy link
Member

vbraun commented Oct 31, 2021

Changed branch from u/mkoeppe/remove__all_import_of_infinity to 97861cd

@vbraun vbraun closed this as completed in 9c23f2a Oct 31, 2021
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
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

3 participants