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

DEPS: drop py2 from test and doc, remove compat #594 #629

Merged
merged 32 commits into from
Mar 30, 2023

Conversation

fangchenli
Copy link
Contributor

@fangchenli fangchenli commented May 22, 2022

close #594

@cclauss
Copy link
Contributor

cclauss commented Sep 6, 2022

Let's not just drop tests without fixing the code. basestring, long, raw_input, xrange were all removed in Python 3.

% flake8 . --count --select=E9,F63,F7,F82,Y --show-source --statistics

./mpmath/ctx_mp_python.py:144:29: F821 undefined name 'long'
    def __long__(s): return long(to_int(s._mpf_))
                            ^
./mpmath/libmp/backend.py:33:11: F821 undefined name 'long'
    MPZ = long
          ^
./mpmath/libmp/backend.py:34:14: F821 undefined name 'xrange'
    xrange = xrange
             ^
./mpmath/libmp/backend.py:35:18: F821 undefined name 'basestring'
    basestring = basestring
                 ^
./demo/mandelbrot.py:27:14: F821 undefined name 'xrange'
    for i in xrange(ITERATIONS):
             ^
./demo/pidigits.py:73:14: F821 undefined name 'raw_input'
    tofile = raw_input("Output to file? (enter a filename, or just press " \
        "enter\nto print directly to the screen) \n> ")
             ^
./demo/pidigits.py:79:5: F821 undefined name 'raw_input'
    raw_input("\nPress enter to close this script.")
    ^

https://portingguide.readthedocs.io

.github/workflows/test.yml Outdated Show resolved Hide resolved
@skirpichev
Copy link
Collaborator

@fangchenli, unfortunately, there are test failures (mostly/all? related to range function vs kwarg conflict), please fix this.
See also previous attempt #597. I think, you left some obsolete stuff (e.g. in docs/basics.rst or docs/functions/trigonometric.rst).

Copy link
Collaborator

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, except for comments above.

@skirpichev skirpichev self-assigned this Mar 25, 2023
@fangchenli fangchenli changed the title DEPS: drop py2 from test and doc #594 DEPS: drop py2 from test and doc, remove compat #594 Mar 25, 2023
@fredrik-johansson
Copy link
Collaborator

Hold on; this patch contains does a bunch of stuff beyond compatibility fixes, changing code like

if ttype is x:

into

if isinstance(t, x):

in performance-critical places (arithmetic operations). The isinstance calls are avoided on purpose here as they are much more expensive than is checks, or at least they used to be much more expensive in older versions of Python. Maybe newer Pythons made isinstance really cheap, but the code should not be changed without profiling to verify this, and such a change should go in a separate PR.

@cclauss
Copy link
Contributor

cclauss commented Mar 25, 2023

isinstance() is faster on modern Python.

% python3.11 -m timeit "type(1) == int"

20000000 loops, best of 5: 17.4 nsec per loop

% python3.11 -m timeit "isinstance(1, int)"

20000000 loops, best of 5: 16.6 nsec per loop

@fredrik-johansson
Copy link
Collaborator

fredrik-johansson commented Mar 25, 2023

OK, but in the original code type() is called only once while the new code may call isinstance() several times, and the operator is is, not ==. And are you sure Python 3.11 doesn't constant-fold your examples?

Anyway, what you'd need to profile is actual mpmath code, not the atomic Python operations.

@cclauss
Copy link
Contributor

cclauss commented Mar 25, 2023

5000000 loops, best of 5: 91.5 nsec per loop # Python 3.7
5000000 loops, best of 5: 63.9 nsec per loop
5000000 loops, best of 5: 92.8 nsec per loop # Python 3.8
5000000 loops, best of 5: 49.6 nsec per loop
5000000 loops, best of 5: 79.8 nsec per loop # Python 3.9
5000000 loops, best of 5: 54.5 nsec per loop
5000000 loops, best of 5: 55.5 nsec per loop # Python 3.10
5000000 loops, best of 5: 55 nsec per loop
10000000 loops, best of 5: 30.9 nsec per loop # Python 3.11
10000000 loops, best of 5: 26.9 nsec per loop

GitHub Action: .github/workflows/type_benchmark.yml

name: type_benchmark
on: [push]
jobs:
  type_benchmark:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/setup-python@v4
        with:
          python-version: |
            3.7
            3.8
            3.9
            3.10
            3.11
      - env:
          OLD: "type(1) == int"
          NEW: "isinstance(1, int)"
        run: |
          python3.7  -m timeit "$OLD"
          python3.7  -m timeit "$NEW"
          python3.8  -m timeit "$OLD"
          python3.8  -m timeit "$NEW"
          python3.9  -m timeit "$OLD"
          python3.9  -m timeit "$NEW"
          python3.10 -m timeit "$OLD"
          python3.10 -m timeit "$NEW"
          python3.11 -m timeit "$OLD"
          python3.11 -m timeit "$NEW"

is vs. == only seems to help in Python 3.11

@fredrik-johansson
Copy link
Collaborator

Timing some mixed mpf arithmetic operations, there doesn't seem to be any measurable impact on performance anymore.

So I'll withdraw my objection, considering that the new version of the code is simpler / more idiomatic.

@skirpichev
Copy link
Collaborator

@fredrik-johansson, from simple benchmarks it looks like isinstance() calls slightly slower:

$ python -m timeit -r11 'isinstance(1, int)'
5000000 loops, best of 11: 90.5 nsec per loop
$ python -m timeit -r11 'type(1) == int'
5000000 loops, best of 11: 88.9 nsec per loop
$ python -m timeit -r11 'type(1) is int'
5000000 loops, best of 11: 74.1 nsec per loop
$ python -m timeit -r11 -s 'it=(int,)' 'type(1) in it'
5000000 loops, best of 11: 84.5 nsec per loop
$ python --version
Python 3.11.1+

The other point is that this change is totally unrelated to the subject of this pr (which is big enough). There is nothing wrong with the old code from the py3 point of view, so I would suggest to revert that part.

Copy link
Collaborator

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr still lacks some changes from #597. So far:

  • docs/functions/trigonometric.rst
  • mpmath/ctx_fp.py (real/imag properties)
  • mpmath/ctx_iv.py (numbers abc)
  • mpmath/functions/functions.py (real/imag properties)
  • mpmath/functions/zeta.py (py2.4 try block)
  • mpmath/libmp/libmpf.py (from_float)
  • mpmath/math2.py (WA for log/sqrt and real/imag)
  • mpmath/rational.py (numbers abc)
  • mpmath/tests/torture.py (multiprocessing)

Lets also revert isinstance() changes. This doesn't belongs to the subject of the pr.

mpmath/libmp/backend.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
docs/basics.rst Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@skirpichev skirpichev requested a review from cclauss March 26, 2023 07:46
mpmath/rational.py Outdated Show resolved Hide resolved
@skirpichev
Copy link
Collaborator

mpmath/libmp/libmpf.py, mpmath/ctx_mp.py, mpmath/matrices/matrices.py and mpmath/tests/test_matrices.py still have workarounds for py2 (or py3<3.5). Try grep:)

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Let's drop the legacy!!!

Copy link
Contributor

@cbm755 cbm755 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! I made a few comments above, only one of any consequence (the dropping of a finally: ctx.prec = prec) block.

mpmath/ctx_iv.py Outdated Show resolved Hide resolved
mpmath/math2.py Outdated Show resolved Hide resolved
mpmath/math2.py Outdated Show resolved Hide resolved
mpmath/functions/zeta.py Show resolved Hide resolved
fangchenli and others added 3 commits March 28, 2023 23:12
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@skirpichev
Copy link
Collaborator

@cbm755, thanks! Lets merge this tomorrow.

@skirpichev skirpichev merged commit dfc2892 into mpmath:master Mar 30, 2023
@skirpichev
Copy link
Collaborator

@fangchenli, thanks

@fangchenli fangchenli deleted the drop-py2-from-test branch March 30, 2023 02:03
@skirpichev skirpichev removed their assignment May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop Python 2 support
5 participants