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

gh-102837: few coverage nitpicks for the math module #102523

Merged
merged 20 commits into from
Sep 3, 2023

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Mar 8, 2023

  • input checks for math_1(L989), math_1a(L1023), math_2(L1064,L1071), hypot(L2682), log(L2307), ldexp(L2168), ceil(L1165), floor(L1236,L1239) and dist(L2587,L2588,L2628).
  • drop inaccessible "if" branch (L3518) in perm_comb_small()
  • improve fsum coverage for exceptional cases (L1433,L1438,L1451,L1497), ditto fmod(L2378)
  • rewrite modf to fix inaccessible case(L2229), ditto for pow(L2988)

(all line numbers are wrt the main branch at 5e6661b)

@skirpichev
Copy link
Member Author

skirpichev commented Mar 8, 2023

Note, c.f. #102067 for the cmath module - there are still some uncovered lines/branches.

@skirpichev
Copy link
Member Author

@AlexWaygood, do you think this requires an issue?

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 8, 2023

@AlexWaygood, do you think this requires an issue?

Our policy is generally to require issues for all changes, unless they're extremely trivial (such as typo fixes).

(If the maintainers of the math module decide this doesn't need an issue then that's obviously fine, but I'm not a math module maintainer, so I don't want to decide that for them :)

@skirpichev
Copy link
Member Author

skirpichev commented Mar 8, 2023 via email

@mdickinson mdickinson self-requested a review March 8, 2023 17:40
- input checks for math_1(L989), math_1a(L1023), math_2(L1064,L1071),
  hypot(L2682), log(L2307), ldexp(L2168) and dist(L2587,L2588,L2628).
- rewrite math_floor like math_ceil (cover L1239)
- drop inaccessible "if" branch (L3518) in perm_comb_small()
- improve fsum coverage for exceptional cases (L1433,L1438,L1451,L1497),
  ditto fmod(L2378)
- rewrite modf to fix inaccessible case(L2229), ditto for pow(L2988)

(all line numbers wrt the main branch at 5e6661b)
@skirpichev skirpichev changed the title Few coverage nitpicks for the math module gh-102837: few coverage nitpicks for the math module Mar 20, 2023
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks!

Modules/mathmodule.c Show resolved Hide resolved
Modules/mathmodule.c Outdated Show resolved Hide resolved
Modules/mathmodule.c Outdated Show resolved Hide resolved
Modules/mathmodule.c Outdated Show resolved Hide resolved
Modules/mathmodule.c Outdated Show resolved Hide resolved
@skirpichev
Copy link
Member Author

Last commit will cover rest of math_floor()/ceil().

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks for the follow ups! :-)

@skirpichev
Copy link
Member Author

Next pr: #110000

Modules/mathmodule.c Show resolved Hide resolved
Modules/mathmodule.c Show resolved Hide resolved
skirpichev added a commit to skirpichev/cpython that referenced this pull request Oct 6, 2023
encukou pushed a commit to encukou/cpython that referenced this pull request Nov 13, 2023
pythonGH-102523)

(Only the test changes from pythonGH-102523 are cherry-picked)

- input checks for math_1(L989), math_1a(L1023), math_2(L1064,L1071), hypot(L2682), log(L2307), ldexp(L2168), ceil(L1165), floor(L1236,L1239) and dist(L2587,L2588,L2628).
- improve fsum coverage for exceptional cases (L1433,L1438,L1451,L1497), ditto fmod(L2378)

(all line numbers are wrt the main branch at 5e6661b)
@bedevere-app
Copy link

bedevere-app bot commented Nov 13, 2023

GH-112030 is a backport of this pull request to the 3.12 branch.

encukou added a commit that referenced this pull request Nov 13, 2023
… (GH-112030)

* gh-102837: improve test coverage for math module (GH-102523)

(Only the test changes from GH-102523 are cherry-picked)

- input checks for math_1(L989), math_1a(L1023), math_2(L1064,L1071), hypot(L2682), log(L2307), ldexp(L2168), ceil(L1165), floor(L1236,L1239) and dist(L2587,L2588,L2628).
- improve fsum coverage for exceptional cases (L1433,L1438,L1451,L1497), ditto fmod(L2378)

(all line numbers are wrt the main branch at 5e6661b)

* gh-102837: more tests for the math module (GH-111930)

Add tests to improve coverage:

* fsum: L1369, L1379, L1383, L1412
* trunc: L2081
* log: L2267
* dist: L2577, L2579
* hypot: L2632
* sumprod: L2744, L2754, L2774, L2778, L2781, L2785, L2831, L2835, L2838
* pow: L2982
* prod: L3294, L3308, L3318-3330

// line numbers wrt to 9dc4fb8

(cherry picked from commit c61de45)

---------

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
encukou added a commit that referenced this pull request Nov 16, 2023
… (GH-112030) (GH-112041)

[3.12] gh-102837: more tests for the math module (GH-111930)(GH-102523) (GH-112030)

* gh-102837: improve test coverage for math module (GH-102523)

(Only the test changes from GH-102523 are cherry-picked)

- input checks for math_1(L989), math_1a(L1023), math_2(L1064,L1071), hypot(L2682), log(L2307), ldexp(L2168), ceil(L1165), floor(L1236,L1239) and dist(L2587,L2588,L2628).
- improve fsum coverage for exceptional cases (L1433,L1438,L1451,L1497), ditto fmod(L2378)

(all line numbers are wrt the main branch at 5e6661b)

* gh-102837: more tests for the math module (GH-111930)

Add tests to improve coverage:

* fsum: L1369, L1379, L1383, L1412
* trunc: L2081
* log: L2267
* dist: L2577, L2579
* hypot: L2632
* (not cherry-picked for 3.11: sumprod)
* pow: L2982
* prod: L3294, L3308, L3318-3330

// line numbers wrt to 9dc4fb8

(cherry picked from commit c61de45)

---------

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
(cherry picked from commit c6aea46)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants