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-37822: Add math.as_integer_ratio(). by serhiy-storchaka · Pull Request #15210 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Aug 11, 2019 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

https://bugs.python.org/issue37822

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
bpo-37822: Add math.as_integer_ratio().
482b1f9
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Aug 11, 2019
@serhiy-storchaka
serhiy-storchaka requested review from mdickinson and rhettinger August 11, 2019 13:57
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 11, 2019
@bedevere-bot bedevere-bot added the awaiting core review label Aug 11, 2019
@serhiy-storchaka
Better TypeError message.
a587d7d
@jdemeyer

jdemeyer commented Aug 12, 2019

Copy link
Copy Markdown
Contributor

I think it would be good to define more precisely what are the output types. We require the output of as_integer_ratio to be two integers, but what does "integer" mean in this context? A Python int? An object which implements __index__? An instance of numbers.Integral?

My personal vote would be to require __index__ (I don't mean that we should check that condition, but we should document it).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jdemeyer jdemeyer mentioned this pull request Aug 19, 2019
bpo-37884: Optimize Fraction() and statistics.mean() #15329
Closed
brandtbucher
brandtbucher requested changes Aug 23, 2019
View reviewed changes

@brandtbucher brandtbucher 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.

One small issue I noticed:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/mathmodule.c
math.as_integer_ratio
x: object
/
greatest common divisor of x and y

@brandtbucher brandtbucher Aug 23, 2019

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.

This looks like a copy-paste error. Maybe:

Suggested change
greatest common divisor of x and y
Return the rational representation of x as the pair (numerator, denominator).

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka serhiy-storchaka Aug 24, 2019

Copy link
Copy Markdown
Member Author

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.

Thank you. But this proposition was rejected.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka serhiy-storchaka closed this Aug 24, 2019
@serhiy-storchaka
serhiy-storchaka deleted the math-as_integer_ratio branch August 24, 2019 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher requested changes
@mdickinson mdickinson Awaiting requested review from mdickinson
@rhettinger rhettinger Awaiting requested review from rhettinger

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review type-feature A feature request or enhancement

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.

5 participants

@serhiy-storchaka @jdemeyer @brandtbucher @the-knights-who-say-ni @bedevere-bot
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.