Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(18)

#24270: PEP 485 (math.isclose) implementation

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by ncoghlan
Modified:
4 years, 9 months ago
Reviewers:
taleinat, berker.peksag, yselivanov
CC:
rhettinger, pmoore, mark.dickinson, Nick Coghlan, scoder, taleinat, stutzbach, Chris.Barker_noaa.gov, skrah, devnull_psf.upfronthosting.co.za, Yury Selivanov
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 30
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/math.rst View 1 1 chunk +32 lines, -0 lines 6 comments Download
Doc/whatsnew/3.5.rst View 1 2 chunks +20 lines, -0 lines 6 comments Download
Lib/test/test_math.py View 1 1 chunk +101 lines, -0 lines 10 comments Download
Misc/NEWS View 1 1 chunk +3 lines, -0 lines 0 comments Download
Modules/mathmodule.c View 1 2 chunks +82 lines, -0 lines 8 comments Download

Messages

Total messages: 4
taleinat_gmail.com
http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py File Lib/test/test_math.py (right): http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py#newcode1128 Lib/test/test_math.py:1128: # # test with integer values Oops! One too ...
4 years, 9 months ago #1
berkerpeksag
Thanks for the patch! Added some trivial comments. http://bugs.python.org/review/24270/diff/14937/Doc/library/math.rst File Doc/library/math.rst (right): http://bugs.python.org/review/24270/diff/14937/Doc/library/math.rst#newcode113 Doc/library/math.rst:113: .. ...
4 years, 9 months ago #2
Yury Selivanov
Just a couple of nits http://bugs.python.org/review/24270/diff/14937/Doc/whatsnew/3.5.rst File Doc/whatsnew/3.5.rst (right): http://bugs.python.org/review/24270/diff/14937/Doc/whatsnew/3.5.rst#newcode284 Doc/whatsnew/3.5.rst:284: PEP 485: A Function ...
4 years, 9 months ago #3
taleinat_gmail.com
4 years, 9 months ago #4
I've gone over all of the comments and implemented appropriate changes in a
revised patch.

http://bugs.python.org/review/24270/diff/14937/Doc/library/math.rst
File Doc/library/math.rst (right):

http://bugs.python.org/review/24270/diff/14937/Doc/library/math.rst#newcode113
Doc/library/math.rst:113: .. function:: isclose(a, b, rel_tol=1e-09,
abs_tol=0.0)
On 2015/05/26 00:01:24, berkerpeksag wrote:
> I think we can make rel_tol and abs_tol keyword-only arguments.

Done.

http://bugs.python.org/review/24270/diff/14937/Doc/library/math.rst#newcode131
Doc/library/math.rst:131: ``abs(a-b) <= max( rel_tol * max(abs(a), abs(b)),
abs_tol )``.
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Whitespace here:
> 
>     abs_tol )``

Done.

http://bugs.python.org/review/24270/diff/14937/Doc/library/math.rst#newcode142
Doc/library/math.rst:142: :pep:`485` -- A Function for testing approximate
equality
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Function -> function

Done.

http://bugs.python.org/review/24270/diff/14937/Doc/whatsnew/3.5.rst
File Doc/whatsnew/3.5.rst (right):

http://bugs.python.org/review/24270/diff/14937/Doc/whatsnew/3.5.rst#newcode284
Doc/whatsnew/3.5.rst:284: PEP 485: A Function for testing approximate equality
On 2015/05/26 17:20:02, Yury.Selivanov wrote:
> "function"

Done.

http://bugs.python.org/review/24270/diff/14937/Doc/whatsnew/3.5.rst#newcode292
Doc/whatsnew/3.5.rst:292: The IEEE 754 special values of ``NaN``, ``inf``, and
``-inf`` will be handled
On 2015/05/26 17:20:02, Yury.Selivanov wrote:
> I don't think you need this paragraph in whatsnew.

Done.

http://bugs.python.org/review/24270/diff/14937/Doc/whatsnew/3.5.rst#newcode595
Doc/whatsnew/3.5.rst:595: (:pep:`485` written and code contributed by Chris
Barker.
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Current format:
> 
> (Contributed by Chris Barker and Tal Einat in :issue:`24270`.)
> 
> We are already mentioning PEP 485 in the whatsnew document and the
math.isclose
> documentation.

Done.

http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py
File Lib/test/test_math.py (right):

http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py#newcode1058
Lib/test/test_math.py:1058: def do_close_all(examples, *args, **kwargs):
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Also, here is a subjective comment: The helpers make the tests hard to
> understand :) If you prefer to keep them, can you add "assert" to their names
> (e.g. assertIsClose)?

Done (changed to assertIsClose).

http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py#newcode1059
Lib/test/test_math.py:1059: for a, b in examples:
On 2015/05/26 00:01:24, berkerpeksag wrote:
> You can use self.subTest here.

I'm not sure I like doing this inside a helper function used by so many tests.
Meanwhile, the basic helpers used by this give informative messages on failure,
which is the most important thing IMO.

http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py#newcode1068
Lib/test/test_math.py:1068: with self.assertRaises(ValueError):
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Could you split these tests into smaller and specific test cases?

Done.

http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py#newcode1128
Lib/test/test_math.py:1128: # # test with integer values
On 2015/05/25 22:21:07, taleinat wrote:
> Oops! One too many comment signs.

Done.

http://bugs.python.org/review/24270/diff/14937/Lib/test/test_math.py#newcode1135
Lib/test/test_math.py:1135: from decimal import Decimal
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Decimal and Faction tests can also be moved to new test cases.

Done.

http://bugs.python.org/review/24270/diff/14937/Modules/mathmodule.c
File Modules/mathmodule.c (right):

http://bugs.python.org/review/24270/diff/14937/Modules/mathmodule.c#newcode1993
Modules/mathmodule.c:1993: /* isclose, as proposed in PEP 485: A Function for
testing approximate equality
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Nitpick: We can remove this comment.

Done.

http://bugs.python.org/review/24270/diff/14937/Modules/mathmodule.c#newcode2057
Modules/mathmodule.c:2057: ":param a: one of the values to be tested\n"
On 2015/05/26 00:01:24, berkerpeksag wrote:
> We don't use Sphinx/docutils-style syntax in the stdlib. I think we can keep
> this simple.

Done (changed to the style of parameter documentation generated by Argument
Clinic).

http://bugs.python.org/review/24270/diff/14937/Modules/mathmodule.c#newcode2065
Modules/mathmodule.c:2065: "Returns True if a is close in value to b, and False
otherwise.\n"
On 2015/05/26 00:01:24, berkerpeksag wrote:
> Returns -> Return

Done.

http://bugs.python.org/review/24270/diff/14937/Modules/mathmodule.c#newcode2071
Modules/mathmodule.c:2071: "See PEP 485 for a detailed description.");
On 2015/05/26 00:01:24, berkerpeksag wrote:
> We can remove this line, too.

Done.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+