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

Deprecate the import of some development-related names from the global namespace #34259

Closed
jhpalmieri opened this issue Aug 1, 2022 · 13 comments

Comments

@jhpalmieri
Copy link
Member

Part of #25383

Component: misc

Author: John Palmieri

Branch/Commit: u/jhpalmieri/trac34259-deprecate-some-imports @ 88940c1

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

@jhpalmieri jhpalmieri added this to the sage-9.7 milestone Aug 1, 2022
@jhpalmieri
Copy link
Member Author

@jhpalmieri
Copy link
Member Author

Commit: aa8a0d2

@jhpalmieri
Copy link
Member Author

comment:2

This branch deprecates a bunch of import statements in sage.misc.all. It leaves intact cputime and walltime, but it changes some doctests by explicitly importing them, in case we do eventually want to deprecate them. There are one or two places where cputime was called but not used, and those have been removed. The branch also changes from sage.all import * in sage.tests.benchmark to explicit import statements.


New commits:

b20ee44trac 34259: deprecate some imports from sage.misc.all
aa8a0d2trac 34259: clarify imports in sage.tests.benchmark

@videlec
Copy link
Contributor

videlec commented Aug 1, 2022

comment:3

The pyflakes report contains a lot of issues, especially related to benchmark.py.

@jhpalmieri
Copy link
Member Author

comment:4

Replying to @videlec:

The pyflakes report contains a lot of issues, especially related to benchmark.py.

Thank you for pointing that out. I can't work on this for a few days, but most of these should be easy to deal with.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 1, 2022

comment:5

Regarding cputime and walltime, I think they are in the global namespace for interactive use for timing, like Matlab's tic/toc.

Instead of just deprecating them, I think we should find out what the standard idiom to do this in modern Python is

@jhpalmieri
Copy link
Member Author

comment:6

Instead of cputime and walltime, I have always used %time and %timeit. I'm sure there are other alternatives.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2022

Changed commit from aa8a0d2 to 88940c1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2022

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

88940c1trac 34259: fix imports and other pyflakes warnings for tests/benchmark.py

@jhpalmieri
Copy link
Member Author

comment:8

Of course %time and %timeit are for interactive use. Within a program, maybe we could try time.process_time (or process_time_ns), either to rewrite cputime or to replace it. timeit.default_timer could be a replacement for walltime. See https://stackoverflow.com/questions/15176619/timing-the-cpu-time-of-a-python-program in addition to the Python docs. But I think this should be on another ticket.

@jhpalmieri
Copy link
Member Author

comment:9

Anyway, I've fixed most of the pyflakes warnings. The remaining one, src/sage/misc/banner.py:71:9 'sage.all' imported but unused, comes from these lines, and I don't think they need to be changed:

    try:
        import sage.all
        have_sage_all = True
    except ImportError:
        have_sage_all = False

@jhpalmieri
Copy link
Member Author

I revived the branch in the pull request at #35841.

vbraun pushed a commit that referenced this issue Jul 20, 2023
gh-35841: deprecate some imports
    
- Head toward deprecation of automatic imports of some dev tools
- Remove unnecessary assignments in tests/benchmark.py

Deprecate the imports of some dev tools

<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

This is a branch to address #34259, replacing the old trac branch. It
doesn't make all of the original changes: the original branch. In
particular, the original branch added a bunch of lines like `from
sage.misc.misc import cputime` even though the import of `cputime` was
not changes, just in case someone wanted to change it later. I skipped
all of those additions. The original branch made a few other changes
that were not connected to the purposes of this pull request (e.g. the
changes to src/sage/combinat/composition.py), and I skipped those, too.

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35841
Reported by: John H. Palmieri
Reviewer(s): Matthias Köppe
@jhpalmieri
Copy link
Member Author

I think this can be closed: the main issues were addressed in #35841, and whatever's left is not worth keeping.

@mkoeppe mkoeppe closed this as not planned Won't fix, can't repro, duplicate, stale Sep 27, 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