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: Increase test coverage for the math module #110000

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Sep 28, 2023

  • 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...

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
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.

@skirpichev
Copy link
Contributor 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.

Lib/test/test_math.py Outdated Show resolved Hide resolved
Lib/test/test_math.py Outdated Show resolved Hide resolved
Lib/test/test_math.py Outdated Show resolved Hide resolved
Lib/test/test_math.py Outdated Show resolved Hide resolved
Lib/test/test_math.py Outdated Show resolved Hide resolved
@serhiy-storchaka
Copy link
Member

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

@skirpichev
Copy link
Contributor 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.

Remove redundant test (python#111416)
@encukou
Copy link
Member

encukou commented Nov 3, 2023

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?

@skirpichev
Copy link
Contributor 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
Copy link
Member

encukou commented Nov 9, 2023

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!

@skirpichev
Copy link
Contributor Author

@encukou, tests now added in #111930

Modules/mathmodule.c Outdated Show resolved Hide resolved
@skirpichev skirpichev marked this pull request as draft December 9, 2023 05:21
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.

None yet

3 participants