[Python-Dev] PEP 485 isclose() implementation review requested

[Chris Barker]

Folks,

After a huge delay, I finally found the time to implement the PEP 485
isclose() function, in C. I tihnk it's time for some review. I appologise
for the fact that I have little experience with C, and haven't used the raw
C API for years, but it's a pretty simple function, and there's lots of
code to copy, so I think it's in OK shape. I hav not yet integrated it with
the cPyton source code -- it belongs in mathmodule.c, but I found it easier
to put it in a separate module while figuring it out.

You can find the code in the same gitHub repo as the draft PEP and python
prototyping code:

https://github.com/PythonCHB/close_pep

the code is in:
  is_close_module.c

There is a test module in:
  test_isclose_c.py

and it can be built with:

python3 setup.py build_ext --inplace

Let me know if I should restructure it or put it anywhere else before it
gets reviewed but in the meantime, i welcome any feedback.

Thanks,
  -Chris

A few questions I have off the bat:

C-API (and plain C) questions:
=============================

* Is there a better way to create a False or True than::

    PyBool_FromLong(0) and PyBool_FromLong(1)

* Style question: should I put brackets in an if clause that has only one
line?::

    if (some_check) {
        just_this_one_expression
    }

* I can't find docs for PyDoc_STRVAR: but it looks like it should use it --
how?

* I'm getting a warning in my PyMethodDef clause::

    static PyMethodDef IsCloseMethods[] = {
        {"isclose", isclose_c, METH_VARARGS | METH_KEYWORDS,
         "determine if two floating point numbers are close"},
        {NULL, NULL, 0, NULL}        /* Sentinel */
    };

is_close_module.c:61:17: warning: incompatible pointer types initializing
      'PyCFunction' (aka 'PyObject *(*)(PyObject *, PyObject *)') with an
      expression of type 'PyObject *(PyObject *, PyObject *, PyObject *)'
      [-Wincompatible-pointer-types]
    {"isclose", isclose_c, METH_VARARGS | METH_KEYWORDS,

but it seems to be working -- and I've copied, as well as I can, the
examples.

Functionality Questions:
========================

* What do do with other numeric types?
  - Integers cast fine...
  - Decimal  and Fraction cast fine, too -- but precision is presumably
lost.
  - Complex ? -- add it to cmath?


* It's raising an Exception for negative tolerances: which don't make sense,
  but don't really cause harm either (fabs() is used anyway). I can't say I
recall why I did that
  for the python prototype, but I reproduced in the C version. Should I?

* What about zero tolerance? should equal values still be considered close?
They are now, and tests reflect that.












-- 

Christopher Barker, Ph.D.
Oceanographer

Emergency Response Division
NOAA/NOS/OR&R            (206) 526-6959   voice
7600 Sand Point Way NE   (206) 526-6329   fax
Seattle, WA  98115       (206) 526-6317   main reception

Chris.Barker at noaa.gov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20150517/bdd265e1/attachment.html>