Skip to content

Conversation

@skirpichev
Copy link
Member

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

Copy link
Member

@serhiy-storchakaserhiy-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
MemberAuthor

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.

@serhiy-storchaka
Copy link
Member

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

@skirpichev
Copy link
MemberAuthor

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

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
MemberAuthor

@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

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
MemberAuthor

@encukou, tests now added in #111930

@skirpichevskirpichev marked this pull request as draft December 9, 2023 05:21
@mdickinson
Copy link
Member

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

@serhiy-storchakaserhiy-storchaka removed the request for review from mdickinsonAugust 13, 2024 18:22
@skirpichevskirpichev marked this pull request as ready for review October 25, 2024 05:07
@skirpichev
Copy link
MemberAuthor

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

@skirpichevskirpichev deleted the math-cov branch October 25, 2024 05:13
Sign up for freeto 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.

4 participants

@skirpichev@serhiy-storchaka@encukou@mdickinson