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 — gh-102837: Increase test coverage for the math module by skirpichev · Pull Request #110000 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented Sep 28, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Improve math_2() comment (mention real use-cases for math_2)
  • trunc: drop _PyType_IsReady() check (L2071) like floor/ceil
  • loghelper: drop inaccessible cases L2234, L2241. Here arg is nonnegative.
  • modf: more efficient (for finite x) handling of special cases (rewrite changes in gh-102837: few coverage nitpicks for the math module #102523)
  • pow

Line numbers are wrt to mathmodule.c at 54fbfa8d5e.


This is a continuation of #102523. There are still some uncovered lines/branches, so issue will not be closed. I can add more commits, but I feel it will be more difficult to review...

  • Issue: Improve test coverage for the math module #102837

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added the awaiting review label Sep 28, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Sep 28, 2023
Improve test coverage for the math module #102837
Closed
@skirpichev skirpichev mentioned this pull request Sep 28, 2023
gh-102837: few coverage nitpicks for the math module #102523
Merged
skirpichev added 12 commits September 28, 2023 07:24
@skirpichev
pythongh-102837: Increase test coverage for the math module
a1d2c1a
* fsum: L1367, L1377, L1381, L1410

// line numbers wrt to 54fbfa8
@skirpichev
Improve math_2() comment (mention real use-cases for math_2)
03e8ead
@skirpichev
* trunc: drop _PyType_IsReady() check (L2071) like floor/ceil, L2079
26489bb
@skirpichev
* log: L2265
128c81d
@skirpichev
* loghelper: drop inaccessible cases L2234, L2235, L2241. Here arg is…
ead9eee
… nonnegative and PyLong_AsDouble raises only OverflowError.
@skirpichev
* dist: L2575, L2577
3a80c78
@skirpichev
* hypot: L2630
8b3bb52
@skirpichev
* sumprod: L2742, L2752, L2772, L2775, L2779, L2783
4e25c35
@skirpichev
Fix typo in sumprod()
7956447
@skirpichev
* sumprod: L2829, L2833, L2836
052d7de
@skirpichev
* pow: L2979, L2980
b431805
@skirpichev
* prod: L3292, L3306, L3316-3328
a046568
@skirpichev
skirpichev force-pushed the math-cov branch from 3dcaaef to a046568 Compare September 28, 2023 04:26
@skirpichev
Merge branch 'main' into math-cov
d7675d7
@skirpichev skirpichev mentioned this pull request Oct 6, 2023
test_math.test_sumprod_stress() takes 5 minutes on debug build #110327
Closed
@skirpichev
Merge branch 'main' into math-cov
96f99cd
serhiy-storchaka
serhiy-storchaka reviewed Oct 6, 2023
View reviewed changes
Comment thread Modules/mathmodule.c Resolved

Uh oh!

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

Comment thread Modules/mathmodule.c Resolved

Uh oh!

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

Comment thread Modules/mathmodule.c Resolved

Uh oh!

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

Comment thread Modules/mathmodule.c Resolved

Uh oh!

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

serhiy-storchaka
serhiy-storchaka reviewed Oct 6, 2023
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.

Also, I think that one of changes in your previous PR should be reverted.

Sorry, something went wrong.

Uh oh!

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

All reactions
skirpichev added 2 commits October 6, 2023 12:15
@skirpichev
More efficient (for finite x) handling of special cases in math.modf
3df127e
Rewrite changes in python#102523
@skirpichev
Change error tests in loghelper() to more lightweight versions (x == …
5e6d59f
…-1.0)
@skirpichev

skirpichev commented Oct 6, 2023

Copy link
Copy Markdown
Member Author

Also, I think that one of changes in your previous PR should be reverted.

Thanks for a suggestion. I did more efficient rewrite of that part, as discussed in the thread.

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev
skirpichev requested a review from serhiy-storchaka October 9, 2023 04:18
serhiy-storchaka
serhiy-storchaka reviewed Oct 15, 2023
View reviewed changes
Comment thread Lib/test/test_math.py Resolved

Uh oh!

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

Comment thread Lib/test/test_math.py Resolved

Uh oh!

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

Comment thread Lib/test/test_math.py Resolved

Uh oh!

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

Comment thread Lib/test/test_math.py Resolved

Uh oh!

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

Comment thread Lib/test/test_math.py Resolved

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented Oct 15, 2023

Copy link
Copy Markdown
Member

How did you write new tests? They look wildly random to me.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka
serhiy-storchaka requested review from mdickinson and rhettinger October 15, 2023 10:25
@skirpichev
Explicit tests for non-float objects (amend a1d2c1a)
3e8d96f
@skirpichev
Amend 8b3bb52 (use math.sqrt(2) instead)
35db224
@skirpichev

skirpichev commented Oct 15, 2023

Copy link
Copy Markdown
Member Author

How did you write new tests? They look wildly random to me.

I prefer exact results, if possible. Then I try to use arch-independent values (e.g. "big enough" integers to trigger overflows on all platforms. And they should hit mentioned lines. Otherwise, they are random.

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Oct 26, 2023
Typo in math_sumprod_impl() #111342
Closed
Useless code block in math.trunc() ? #111417
Closed
@skirpichev
Amend 052d7de
6844b19
Remove redundant test (python#111416)
@encukou

encukou commented Nov 3, 2023

Copy link
Copy Markdown
Member

This PR combines

  • test improvements, which should be backported
  • refactoring (dead code removal), which IMO should not be backported, and
  • possibly a bugfix, which should be backported together with a previously failing test

To make the backporting easier, would it make sense to split the PR along those lines?

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev
Merge branch 'main' into math-cov
6037d84
@skirpichev

skirpichev commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

@encukou, sorry, I wasn't aware that tests are systematically backported (based on #102067 and #102523).

possibly a bugfix, which should be backported together with a previously failing test

Probably, you are about typo in sumprod(). This is fixed and backported, see #111342. Or something else?

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@encukou

encukou commented Nov 9, 2023

Copy link
Copy Markdown
Member

The backporting is a fairly new policy.

But, another benefit of splitting this would be that the test additions could be merged right now, without waiting for @mdickinson to comment on the removed code :)

Thanks for the typo fix!

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev

skirpichev commented Nov 10, 2023

Copy link
Copy Markdown
Member Author

@encukou, tests now added in #111930

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev skirpichev mentioned this pull request Nov 10, 2023
gh-102837: more tests for the math module #111930
Merged
encukou and others added 2 commits November 13, 2023 11:55
@encukou
Merge branch 'main' into math-cov
d2c5170
@serhiy-storchaka
Merge branch 'main' into math-cov
833d852
skirpichev
skirpichev commented Dec 8, 2023
View reviewed changes
Comment thread Modules/mathmodule.c Resolved

Uh oh!

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

skirpichev added 2 commits December 9, 2023 06:20
@skirpichev
Merge branch 'main' into math-cov
5768395
@skirpichev
Revert PyErr_ExceptionMatches
ad84c4a
@skirpichev
skirpichev marked this pull request as draft December 9, 2023 05:21
@bedevere-app bedevere-app Bot removed the awaiting review label Dec 9, 2023
skirpichev added 3 commits April 2, 2024 11:41
@skirpichev
Merge branch 'main' into math-cov
21bc1b8
@skirpichev
Merge branch 'main' into math-cov
d488a1d
@skirpichev
Merge branch 'main' into math-cov
dce58b3
@mdickinson

mdickinson commented Aug 13, 2024

Copy link
Copy Markdown
Member

Please could some kind person remove the request for review for me?

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka
serhiy-storchaka removed the request for review from mdickinson August 13, 2024 18:22
@skirpichev
Merge branch 'main' into math-cov
d313423
@skirpichev skirpichev added the skip news label Oct 7, 2024
@skirpichev
skirpichev marked this pull request as ready for review October 25, 2024 05:07
@bedevere-app bedevere-app Bot added the awaiting review label Oct 25, 2024
@skirpichev

skirpichev commented Oct 25, 2024

Copy link
Copy Markdown
Member Author

@serhiy-storchaka, I'm going to close this (together with the issue) on same ground as #109642. It seems, CPython devs not welcoming now code coverage improvements.

All reactions

Sorry, something went wrong.

Uh oh!

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

@skirpichev skirpichev closed this Oct 25, 2024
@skirpichev
skirpichev deleted the math-cov branch October 25, 2024 05:13
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
@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 review skip news

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

@skirpichev @serhiy-storchaka @encukou @mdickinson
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.