GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-42222: Modernize integer test/conversion in randrange() by rhettinger · Pull Request #23064 · python/cpython

Conversation

@rhettinger

@rhettinger rhettinger commented Oct 31, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Moves the int(x) == x test and conversion into the C code for operator.index().

Provides a 7% speed up:

$ pytime -s 'from random import randrange' 'randrange(15)'  # baseline
500000 loops, best of 11: 540 nsec per loop
$ pytime -s 'from random import randrange' 'randrange(15)'   # new versio
500000 loops, best of 11: 502 nsec per loop

It should become even faster when we get a zero-cost try.

It's odd that non-integers raise a ValueError. I think that should have been TypeError.

https://bugs.python.org/issue42222

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rhettinger
Use index() instead of testing int(x)==x
e11b1dc
@rhettinger rhettinger added the performance Performance or resource usage label Oct 31, 2020
@rhettinger
rhettinger requested review from serhiy-storchaka and tim-one October 31, 2020 17:27
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 31, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Oct 31, 2020
rhettinger added 2 commits October 31, 2020 10:28
@rhettinger
Add blurb
ebe8391
@rhettinger
Deprecate float arguments and the ValueError
ed2f6fd
serhiy-storchaka
serhiy-storchaka reviewed Oct 31, 2020
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Currently you are on the way to #19112 (which already has tests and documentation and cover corner cases).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 31, 2020
bpo-37319: Deprecated support of non-integer arguments in random.randrange(). #19112
Merged
@github-actions

github-actions Bot commented Dec 16, 2020

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@rhettinger rhettinger removed the stale Stale PR or inactive for long period of time. label Dec 24, 2020
rhettinger added 4 commits December 24, 2020 00:32
@rhettinger
Add more detail to blurb
ab9aeb7
@rhettinger
Add tests
f32d175
@rhettinger
Update the docs
ebcb257
@rhettinger
Improve blurb
832d885
@serhiy-storchaka

serhiy-storchaka commented Dec 24, 2020

Copy link
Copy Markdown
Member

What about #19112?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rhettinger

rhettinger commented Dec 24, 2020

Copy link
Copy Markdown
Contributor Author

I merged in the relevant parts of #19112. The principal differences are:

  • The TypeError won't start being emitted until the after the deprecation period.
  • Improved wording for the doc entries.
  • Tests focus solely on the deprecations.
  • All existing behavior remains intact until after the deprecation.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Dec 24, 2020

Copy link
Copy Markdown
Member
  • The TypeError won't start being emitted until the after the deprecation period.

As in #19112 initially. I changed ValueError to TypeError on your request.

  • Improved wording for the doc entries.

I would appreciate any suggestions to change wording.

  • Tests focus solely on the deprecations.

Yes, tests in #19112 cover more cases.

  • All existing behavior remains intact until after the deprecation.

As well as in #19112.

But this PR still has issues. It does not emit warning for randrange(10, 20, 1.0).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rhettinger added 2 commits December 24, 2020 14:20
@rhettinger
Cover the case with a unit step
9e6945d
@rhettinger
More concise blurb
85cc9ee
@rhettinger
rhettinger merged commit a9621bb into python:master Dec 28, 2020
@bedevere-bot

bedevere-bot commented Dec 28, 2020

Copy link
Copy Markdown

@rhettinger: Please replace # with GH- in the commit message next time. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting core review label Dec 28, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@rhettinger @adorilson
bpo-42222: Modernize integer test/conversion in randrange() (python#2…
12cdba8 
…3064)
@AlexWaygood AlexWaygood mentioned this pull request Feb 4, 2022
bpo-46066: Deprecate kwargs syntax for TypedDict definitions #31126
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@tim-one tim-one Awaiting requested review from tim-one

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@rhettinger @serhiy-storchaka @bedevere-bot @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.