Math isclose - #4894

Which isn't a surprise because there's not that many ways to do this particular comparison

I agree. It's pretty straightforward and (like most math operations) only really one way to do it correctly.

I did not make this feature conditional in the build as it just seems too important,

I tend to agree, but I think this addition will be rather large (didn't test it yet).

I pushed the MP_QSTR_ change, removal of isnan and a .exp file.

Size of master (assuming the output after LINK micropython is what gets used to check code size?):

   text	   data	    bss	    dec	    hex	filename
   5149	   3814	      0	   8963	   2303	build/build/frozen_mpy.o
      2	      0	      0	      2	      2	build/build/frozen.o
 406221	   6008	   2488	 414717	  653fd	micropython

For this commit:

   text	   data	    bss	    dec	    hex	filename
   5149	   3814	      0	   8963	   2303	build/build/frozen_mpy.o
      2	      0	      0	      2	      2	build/build/frozen.o
 406869	   6008	   2488	 415365	  65685	micropython

When I apply the change where the default tolerance values are stored in constant objects the output is exactly the same.

As any other additional feature added to (otherwise relatively self-sufficient) MicroPython, this should be conditional, default off.

@pfalcon

What's the threshold for a genuinely useful feature? Just because it came later somehow makes it less valuable/important than something that came earlier? Just because we wrote a lot of Python without a given feature doesn't make that feature not useful.

Conditional features are completely invisible to the vast majority of MicroPython users who aren't going to build their own firmware. It's the sort of thing that turns away new users. It makes the documentation confusing.

I'd be keen to investigate some sort of "feature levels" so that conditional flags could be grouped together to provide a manageable set of build targets, but in the meantime these sort of comments on every PR that adds a new feature aren't very helpful. Everyone already knows that features (usually) comes at a space cost, and nobody wants to waste bytes, but features should be considered on their merits rather than a blanket ban.

What's the threshold for a genuinely useful feature?

All various criteria were sounded many times. Here I'd suggest a following one: make a list of one thousand features you want to add to MicroPython. Of those, only one can be added. So, choose wisely. Don't make hasty choices. Collecting that list of 1000 features would alone take time, but still don't haste with that most important one. Wait half a year with it, hopefully it will wean off. If not, still wait a year to confirm it's really useful.

This particular PR shows a good example of a feature which is hardly worth the bloat it adds on the C level (which affects everyone, whether they use it or not). What 99% people would need out of it is simple Python expression abs(a - b) < epsilon. Can be made a function easily. And if you want to suggest that current math module should be umath instead, to allow to build such functions as isclose() in Python, in proper math module which can be made as much (or as little) compatible with CPython as needed - then you'd be very right, +1 from me.

Everyone already knows that features (usually) comes at a space cost, and nobody wants to waste bytes, but features should be considered on their merits rather than a blanket ban.

I'm sorry, but I disagree. A typical emotion people seem to feel about MicroPython (without even properly realizing it) is something like: "What a small little cutie, we like it so much! Now, let's turn it into UGLY FAT MONSTER, because heck, we like it so much." I'm sorry, but if you need more features right away, just use CPython. Otherwise, just develop (and share, maintain, etc.) standalone modules in Python, because even CPython doesn't implement "everything" and you need external modules (and majority of CPython stdlib is developed in Python of course).

Oops forgot to ask: is does this fall under 'special' functions i.e. should this be conditional with MICROPY_PY_MATH_SPECIAL_FUNCTIONS or a dedicated macro?

What 99% people would need out of it is simple Python expression abs(a - b) < epsilon

Surely not everyone needs it, but I'm not too sure of that number. Or of the amount of people who can correctly estimate that epsilon to do the right thing for their case.

Surely not everyone needs it, but I'm not too sure of that number.

But the talk started with pointing out that there's the established best practice that newly added stuff is made conditional. And if there's anything more to talk about it, it's a worrying fact that even long-time contributors ignore that best practice regularly. (Unless of course you think that math.isclose() is the crux of Python programming, in which case I'm happy to have countered that it's not.)

ignore that best practice regularly

Wasn't ignored here, rather intentionally done like that.. But it's a PR so if it's not to people's liking they can comment and it can still be added.

but if you need more features right away, just use CPython.

You can't use CPython on a microcontroller, which is the primary aim of MicroPython. And, following (C)Python, MicroPython's aim is to be a useful language. There are other languages (eg Lua) which are minimal languages. Python is not a minimal language. And via the PEP process it was decided that math.isclose() is a worthy addition to Python. So hence it's a worthy contender to put into MicroPython, perhaps enabled by default, perhaps not. Just because MicroPython started at the time CPython 3.4 was out (actually 3.3) doesn't mean that's the final version and it cannot go beyond that.

the established best practice that newly added stuff is made conditional

This is not a good principle to go by because it leads to overly conditional code, too many configuration options, complete fragmentation of new features moving forward, and confusion as to what port supports what feature. And then the inability to use any new feature because it may not be there. Sure there has to be a considered balance of selecting new features to be included by default, but it doesn't make sense to be stuck always at what exists now.

Oops forgot to ask: is does this fall under 'special' functions i.e. should this be conditional with MICROPY_PY_MATH_SPECIAL_FUNCTIONS or a dedicated macro?

It's not really a special function, more like an extra/misc/aux function. There is already MICROPY_PY_MATH_FACTORIAL for individual math.factorial(), so it could be MICROPY_PY_MATH_ISCLOSE. But it feels like math.isclose() is more useful/fundamental than functions like degrees() and radians() which are always there...

I have the same feeling but Paul doesn't so if we can decide what it's going to be I can adjust the code. How about MICROPY_PY_MATH_ISCLOSE which gets enabled for unix/windows/stm32 ports, for instance?

Looking at code which should work the same across various builds and platforms, having build options for parts of a module like math is painful. Build options for complete modules are fine, espcially when it comes to save code size.

For the record the PEP is https://www.python.org/dev/peps/pep-0485/

I computed the size increase with this patch:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +704 +0.141% [incl +64(data)]
unix nanbox:  +640 +0.144% [incl +32(data)]
      stm32:  +352 +0.095% PYBV10
     cc3200:    +0 +0.000% 
    esp8266:  +376 +0.057% 
      esp32:  +316 +0.028% [incl +96(data)]
        nrf:  +352 +0.241% pca10040
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

That's quite a lot, so I suggest this new function be made optional via MICROPY_PY_MATH_ISCLOSE, enabled on those that have already got MATH_SPECIAL_FUNCTIONS: esp32, stm32, unix, windows.

Ok addressed all comments

Thanks! Merged in af5c998 with minor style edits, and additional tests for passing in negative numbers to abs_tol and rel_tol (to get 100% test coverage of the new function).

tests for passing in negative numbers

Good catch, thanks!