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.categories: Remove module-level imports of sage.rings, sage.algebras, sage.matrix, sage.misc.latex, etc. #29873

Closed
mkoeppe opened this issue Jun 16, 2020 · 25 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jun 16, 2020

This is for a cleaner, filtered structure of our Python modules, at least regarding their load time. This is preparation for #29865 (sage-objects).

The idea is that sage.categories should be more abstract than any of the implemented algebraic structures in sage.rings, sage.algebras, sage.modules, ... Therefore it should only have a (module load time) dependence on sage.structure and sage.misc.

The present ticket changes module-level imports to method-level imports so that they are resolved later than at module load time.

At run time, of course, there are additional dependencies. For example, as noted in #29705, the output of .cardinality() must be an integer (ZZ).

Also, obviously the doctests need a larger part of the library.

(split out from #29865)

Depends on #29869
Depends on #16351

CC: @videlec @tscrim @orlitzky @nthiery @timokau @fchapoton

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 27f2dab

Reviewer: Travis Scrimshaw

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jun 16, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 16, 2020

Dependencies: #29869

@mkoeppe mkoeppe changed the title sage.categories: Remove module-level imports of sage.rings, sage.algebras, sage.matrix sage.categories: Remove module-level imports of sage.rings, sage.algebras, sage.matrix, sage.misc.latex Jun 16, 2020
@mkoeppe mkoeppe changed the title sage.categories: Remove module-level imports of sage.rings, sage.algebras, sage.matrix, sage.misc.latex sage.categories: Remove module-level imports of sage.rings, sage.algebras, sage.matrix, sage.misc.latex, etc. Jun 16, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 16, 2020

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 16, 2020

New commits:

907feebMove attrcall and friends from sage.misc.misc to new module sage.misc.call
a5453bfFixup: Add src/sage/misc/call.py
64c5701lazy_import from sage.misc.call with deprecation
65414f7Fix imports and one deprecation warning
b9314d4sage.misc.call: Add standard header information, add to reference manual
5e27414sage.categories.crystals: Make import of sage.misc.latex local to a method
6e0fa7csage.categories: Make imports from sage.rings, .sets, .combinat, .plot, .matrix local to methods

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 16, 2020

Commit: 6e0fa7c

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 16, 2020

comment:6

Branch is on top of #29869.

@tscrim
Copy link
Collaborator

tscrim commented Jun 16, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jun 16, 2020

comment:7

In this code, these extra imports at the method level will not have a meaningful effect on the run time, so I am giving this a positive review based on that. However, this is something to be aware of as there can be functions in the category framework that are used in tighter loops.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:8

Thanks! Yes, let's watch out for performance impacts of some of the upcoming changes.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:9

Actually now I see

  File "/local/sage-patchbot/sage/local/lib/python3.7/site-packages/sage/combinat/backtrack.py", line 76, in <module>
    from sage.sets.recursively_enumerated_set import RecursivelyEnumeratedSet_generic
  File "sage/sets/recursively_enumerated_set.pyx", line 179, in init sage.sets.recursively_enumerated_set (build/cythonized/sage/sets/recursively_enumerated_set.c:12654)
ImportError: cannot import name SearchForest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6024ffdsrc/sage/misc/call.py: register_unpickle_override for call_method
f3afd30sage.categories.crystals: Make import of sage.misc.latex local to a method
6433a56sage.categories: Make imports from sage.rings, .sets, .combinat, .plot, .matrix local to methods

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

Changed commit from 6e0fa7c to 6433a56

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:11

rebased on top of newer #29869

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:12

cyclic import, it seems

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:13

sage.combinat.backtrack notes a relevant todo:

    - For now the code of :class:`SearchForest` is still in
      ``sage/combinat/backtrack.py``.  It should be moved in
      ``sage/sets/recursively_enumerated_set.pyx`` into a class named
      :class:`RecursivelyEnumeratedSet_forest` in a later ticket.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:14

that's apparently #16351.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

Changed dependencies from #29869 to #29869, #16351

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

Changed commit from 6433a56 to 5c20de7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 17, 2020

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

364d7b3Move SearchForest code to sage/sets/recursively_enumerated_set.pyx
68f7bd5sage.combinat.integer_vectors_mod_permgroup: Update to use RecursivelyEnumeratedSet_forest
1764ca4Update remaining uses of SearchForest to use RecursivelyEnumeratedSet_forest
1ac0a09sage.combinat.backtrack: Remove deprecated class SearchForest
908acbasage.combinat.backtrack: Remove deprecated classes TransitiveIdeal and TransitiveIdealGraded
5c20de7Merge branch 't/16351/move_searchforest_code_to_sage_sets_recursively_enumerated_set_pyx' into t/29873/sage_categories_remove_module_level_imports

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 17, 2020

comment:17

Merging #16351 fixed the ImportError.
Ready for review

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2020

comment:19

One last little trivial thing, please add

# -*- coding: utf-8 -*-

as the first line to call.py. Once done, you can set a positive review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2020

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

27f2dabsrc/sage/misc/call.py: Add coding directive

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2020

Changed commit from 5c20de7 to 27f2dab

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 18, 2020

comment:21

Thanks!

@vbraun
Copy link
Member

vbraun commented Jul 2, 2020

Changed branch from u/mkoeppe/sage_categories_remove_module_level_imports to 27f2dab

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