gh-148868: Increase test coverage for cmath.isinf#148869
gh-148868: Increase test coverage for cmath.isinf#148869manoj-km24 wants to merge 2 commits intopython:mainfrom
cmath.isinf#148869Conversation
| self.assertFalse(cmath.isinf(NAN)) | ||
| self.assertTrue(cmath.isinf(INF)) | ||
| self.assertTrue(cmath.isinf(-INF)) | ||
| self.assertFalse(cmath.isinf(INF/INF)) |
There was a problem hiding this comment.
Why this? INF/INF is NaN so it does not make any sense
There was a problem hiding this comment.
INF/INF seems to be an exception because when we take INF + INF and INF * INF , they produce INF but on the other hand INF/INF produces NaN. So I thought it would be worth testing that too . I could change it if you like
There was a problem hiding this comment.
It is not an exception, it is what the standard expects (and what maths tells you about). Adding two infinities or multiplying result in an infinity. However division and subtration of infinities do not have a limit hence a NaN. Please make sure you understand the issue before creating one and opening a PR in the future.
So, remove this case. You can keep the -INF one but I do not really think it is worth. In particular we want to check that isinf from math (and not cmath) also checks negative infinity.
There was a problem hiding this comment.
Will do . Sorry , i misused the word "exception". What I wanted to say was if in case we change the behaviour of INF, would it be better if we check what subtracting or dividing 2 infinities produces , as the standard expects since addition and multiplication of 2 infinities are obvious. Just a doubt ,I will clear the INF/INF case in a minute.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
cmath.isinf
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
|
Do we test math.isinf(-INF)? if not please add that test case as well |
|
And Sergey said that we already cover signed cases elsewhere so I am unsure whether this really adds value. |
As Sergey said , The test is covered in test_math.py (-inf) . Should I keep it in test_cmath.py ? |
|
If test_math tests math.isinf(-inf) then we can indeed add your test. I think we could have cleaner tests with a bit more loops and subtests but this would be a bit churn. |
This PR adds additional test cases for the isinf() function in cmath library.
The test cases are verified from the below snippet: