MAINT: special: refactor error handling #334

Merged
merged 29 commits into from Nov 11, 2012

Conversation

Projects
None yet
2 participants
@pv
Member

pv commented Oct 7, 2012

Refactor error handling in scipy.special. Also make lambertw use the common system.

This goes on top of #333 --- commit 0648b84 and its descendants are of interest here

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Oct 14, 2012

Member

a26f3ab adds checks for dangerous downcasts in input and output, so that for instance yn(1.5, 2) returns nan, rather than yn(1.5, 2) == yn(1, 2).

Member

pv commented Oct 14, 2012

a26f3ab adds checks for dangerous downcasts in input and output, so that for instance yn(1.5, 2) returns nan, rather than yn(1.5, 2) == yn(1, 2).

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 4, 2012

Member

I think this is now more or less converged, and the behavior is as follows:

  • Casting: unsafe casts produce NaNs if they are not exact.
  • Unsafe casts (apart from long->int which is more or less necessary on 64-bit) are not automatically generated. For instance, passing doubles to integer arguments raises an error now.
  • Legacy: however, all old functions that truncated floats to integers retain their previous behavior (= manual special casing)
  • Floating point exception flags are now caught and handled with the same mechanism as the rest of the special function warnings. Previously, these were caught by the usual ufunc machinery, spamming warnings. This is more a nuisance than help --- the special function implementations do not set overflow/invalid flags when returning infs or nans, so the fp exception status cannot be relied on. So I think it is better to make them disabled by default, and allow the user to control them the same way as the rest of the provisional evaluation warnings.
Member

pv commented Nov 4, 2012

I think this is now more or less converged, and the behavior is as follows:

  • Casting: unsafe casts produce NaNs if they are not exact.
  • Unsafe casts (apart from long->int which is more or less necessary on 64-bit) are not automatically generated. For instance, passing doubles to integer arguments raises an error now.
  • Legacy: however, all old functions that truncated floats to integers retain their previous behavior (= manual special casing)
  • Floating point exception flags are now caught and handled with the same mechanism as the rest of the special function warnings. Previously, these were caught by the usual ufunc machinery, spamming warnings. This is more a nuisance than help --- the special function implementations do not set overflow/invalid flags when returning infs or nans, so the fp exception status cannot be relied on. So I think it is better to make them disabled by default, and allow the user to control them the same way as the rest of the provisional evaluation warnings.
@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

Sounds good. Would it make sense to emit warnings for the legacy behavior though? If floats get truncated it's likely a bug in user code.

Member

rgommers commented Nov 4, 2012

Sounds good. Would it make sense to emit warnings for the legacy behavior though? If floats get truncated it's likely a bug in user code.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 4, 2012

Member

Ok, done. These warnings are however not printed by default...

e8f7a02 changes the way error propagation is done. The issue is what happens when "warnings.simplefilter('error', ...)" is in effect, and I ended up deciding to avoid unnecessary PyErr_Occurred calls, over playing completely fair with Cython.

Member

pv commented Nov 4, 2012

Ok, done. These warnings are however not printed by default...

e8f7a02 changes the way error propagation is done. The issue is what happens when "warnings.simplefilter('error', ...)" is in effect, and I ended up deciding to avoid unnecessary PyErr_Occurred calls, over playing completely fair with Cython.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

The default value of special.errprint is documented to be 0, but seems to be 1. And a bigger problem:

In [23]: special.errprint(1)
Out[23]: 0

In [24]: bdtr(1.5, 2.33, 0.5)
/usr/bin/ipython:1: SpecialFunctionWarning: scipy.special/bdtr: (other error) floating point number truncated to integer
  #!/usr/bin/env python
Segmentation fault (core dumped)
Member

rgommers commented Nov 4, 2012

The default value of special.errprint is documented to be 0, but seems to be 1. And a bigger problem:

In [23]: special.errprint(1)
Out[23]: 0

In [24]: bdtr(1.5, 2.33, 0.5)
/usr/bin/ipython:1: SpecialFunctionWarning: scipy.special/bdtr: (other error) floating point number truncated to integer
  #!/usr/bin/env python
Segmentation fault (core dumped)
@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

Do you think turning on those warnings by default will be too noisy? I'd think it's desirable.

Member

rgommers commented Nov 4, 2012

Do you think turning on those warnings by default will be too noisy? I'd think it's desirable.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 4, 2012

Member

Errprint default is 0 --- errprint returns the previous value. But yep, initialization of the variable was missing.

Doesn't segfault for me --- check rebuild & Cython files?

I think turning all special function warnings on by default will be too noisy. The double truncation warnings could in principle handled separately, although the exceptions need to be again smuggled behind Cython's back.

Member

pv commented Nov 4, 2012

Errprint default is 0 --- errprint returns the previous value. But yep, initialization of the variable was missing.

Doesn't segfault for me --- check rebuild & Cython files?

I think turning all special function warnings on by default will be too noisy. The double truncation warnings could in principle handled separately, although the exceptions need to be again smuggled behind Cython's back.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

Sorry, I wrote that down the wrong way around. What I meant to say is, default is documented to be 1. errprint docstring:

Docstring:
errprint(flag)

Sets the error printing flag for special functions (from the
cephesmodule). The output is the previous state.  With errprint(0)
no error messages are shown; the default is errprint(1).  If no
argument is given the current state of the flag is returned and no
change occurs.
Member

rgommers commented Nov 4, 2012

Sorry, I wrote that down the wrong way around. What I meant to say is, default is documented to be 1. errprint docstring:

Docstring:
errprint(flag)

Sets the error printing flag for special functions (from the
cephesmodule). The output is the previous state.  With errprint(0)
no error messages are shown; the default is errprint(1).  If no
argument is given the current state of the flag is returned and no
change occurs.
@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 4, 2012

Member

I think the documentation is wrong --- it seems to have been defaulting to zero since 2003 (a252b05).

Member

pv commented Nov 4, 2012

I think the documentation is wrong --- it seems to have been defaulting to zero since 2003 (a252b05).

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

Rebuilt, still segfaulting. What do you mean by checking Cython files?

Member

rgommers commented Nov 4, 2012

Rebuilt, still segfaulting. What do you mean by checking Cython files?

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

Rebasing this on master would be useful by the way.

Member

rgommers commented Nov 4, 2012

Rebasing this on master would be useful by the way.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

Setting errprint(1) and then running the tests is quite noisy. One issue that I see there is that some function names are replaced by ? in the warning message:

/home/rgommers/Code/scipy/scipy/special/tests/test_basic.py:368: 
SpecialFunctionWarning: scipy.special/?:(underflow) floating point underflow
cephes.obl_rad2(1,1,1,0)
Member

rgommers commented Nov 4, 2012

Setting errprint(1) and then running the tests is quite noisy. One issue that I see there is that some function names are replaced by ? in the warning message:

/home/rgommers/Code/scipy/scipy/special/tests/test_basic.py:368: 
SpecialFunctionWarning: scipy.special/?:(underflow) floating point underflow
cephes.obl_rad2(1,1,1,0)
@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 4, 2012

Member

Also rebuilt, but I didn't manage to reproduce the crash. Something is non-obvious here.

The ? in the warnings are due to the fact that the name of the function is not known to the ufunc loop. This can be worked around, though, if necessary.

Member

pv commented Nov 4, 2012

Also rebuilt, but I didn't manage to reproduce the crash. Something is non-obvious here.

The ? in the warnings are due to the fact that the name of the function is not known to the ufunc loop. This can be worked around, though, if necessary.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

That last GEN commit didn't fix the issue. Here's where it crashes:

test_obl_rad2 (test_basic.TestCephes) ... /home/rgommers/Code/scipy/scipy/special/tests/test_basic.py:368:
SpecialFunctionWarning: scipy.special/?: (underflow) floating point underflow
  cephes.obl_rad2(1,1,1,0)
Segmentation fault (core dumped)
Member

rgommers commented Nov 4, 2012

That last GEN commit didn't fix the issue. Here's where it crashes:

test_obl_rad2 (test_basic.TestCephes) ... /home/rgommers/Code/scipy/scipy/special/tests/test_basic.py:368:
SpecialFunctionWarning: scipy.special/?: (underflow) floating point underflow
  cephes.obl_rad2(1,1,1,0)
Segmentation fault (core dumped)
@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

The ? isn't that important probably, so depends on how much work it is to work around that.

Member

rgommers commented Nov 4, 2012

The ? isn't that important probably, so depends on how much work it is to work around that.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 4, 2012

Member

I'll try to look into the crash more, but I won't have time in the next few days.

Member

rgommers commented Nov 4, 2012

I'll try to look into the crash more, but I won't have time in the next few days.

pv added some commits Nov 3, 2012

ENH: special: restore backwards compatibility for integer-arg functions
Special-case old functions which cast silently their input arguments to
integers. This restores full backwards compatibility.
ENH: special: make SpecialFunctionWarning always emitted by default
This should reduce confusion --- whether the warning is emitted or not
is already user-controlled by errprint(), and the default behavior of
Python warnings may be unexpected.
BUG: special: yet another fix to error propagation
The problem is that sf_error may be called somewhere deep in a special
function implementation, and this may raise a Python exception,
depending on the warnings settings.

If sf_error is defined for Cython as except *, Cython tends to swallow
these exceptions. The alternatives are then either to declare everything
as except *, or nothing as except * --- that is, either to check
PyErr_Occurred() everywhere, or check it nowhere (except after the ufunc
loop exits, Numpy checks it anyway).

For performance reasons (avoid unnecessary synchronization points due to
GIL locking), I'll pick here the latter option --- we raise the
exception without telling Cython about it, and rely on the usual ufunc
machinery collecting it.
@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 4, 2012

Member

Aaand, rebased. Thanks for your help.

Regarding the crash, checking if it occurs at cbada9c would be useful (needs regenerating Cython files, though).

The workaround for function names needs passing a struct with the function pointer and name to the ufunc loops.

Member

pv commented Nov 4, 2012

Aaand, rebased. Thanks for your help.

Regarding the crash, checking if it occurs at cbada9c would be useful (needs regenerating Cython files, though).

The workaround for function names needs passing a struct with the function pointer and name to the ufunc loops.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 11, 2012

Member

Still crashes with cbada9c + regenerating _ufuncs.c and _ufuncs_cxx.cxx.

What I didn't notice before is that it only crashes with numpy 1.6.1 and not with numpy master.

Member

rgommers commented Nov 11, 2012

Still crashes with cbada9c + regenerating _ufuncs.c and _ufuncs_cxx.cxx.

What I didn't notice before is that it only crashes with numpy 1.6.1 and not with numpy master.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 11, 2012

Member

I haven't so far managed to reproduce the crash (also with 1.6.1), and Valgrind doesn't show anything...
Does current Scipy master also crash?

Two additional things could be useful: (i) gdb backtrace (please build with OPT="-ggdb" and FOPT="-ggdb"), and (ii) bisection to the culprit commit.

For bisecting, you might do git bisect run python bisectrun.py -n crashtest.py prebuild.py
with https://github.com/pv/scipy-build-makefile/blob/master/bisectrun.py and

# crashtest.py
import os

# fork to trap segv
pid = os.fork()
if pid == 0:
    # child
    from scipy import special
    special.errprint(1)
    special.bdtr(1.5, 2.33, 0.5)
    os._exit(0)
else:
    pid, status = os.waitpid(pid, 0)
    assert status == 0
# prebuild.py
import os
import sys
import subprocess

os.chdir('scipy/special')
ret = subprocess.call([sys.executable, 'generate_ufuncs.py'])
if ret != 0:
    raise RuntimeError("regen failed!")
Member

pv commented Nov 11, 2012

I haven't so far managed to reproduce the crash (also with 1.6.1), and Valgrind doesn't show anything...
Does current Scipy master also crash?

Two additional things could be useful: (i) gdb backtrace (please build with OPT="-ggdb" and FOPT="-ggdb"), and (ii) bisection to the culprit commit.

For bisecting, you might do git bisect run python bisectrun.py -n crashtest.py prebuild.py
with https://github.com/pv/scipy-build-makefile/blob/master/bisectrun.py and

# crashtest.py
import os

# fork to trap segv
pid = os.fork()
if pid == 0:
    # child
    from scipy import special
    special.errprint(1)
    special.bdtr(1.5, 2.33, 0.5)
    os._exit(0)
else:
    pid, status = os.waitpid(pid, 0)
    assert status == 0
# prebuild.py
import os
import sys
import subprocess

os.chdir('scipy/special')
ret = subprocess.call([sys.executable, 'generate_ufuncs.py'])
if ret != 0:
    raise RuntimeError("regen failed!")
@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 11, 2012

Member

Master works fine with errprint(1). I'll try your suggestions above.

Member

rgommers commented Nov 11, 2012

Master works fine with errprint(1). I'll try your suggestions above.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 11, 2012

Member

Even without reproducing it you managed to fix it with fff0289. Nice!

A note on the bisect scripts: if you re-cythonize some code you need to backup the original file before doing so and then restore it after the build. Otherwise bisect can't switch to the next commit due to changed in a filke under source control.

Member

rgommers commented Nov 11, 2012

Even without reproducing it you managed to fix it with fff0289. Nice!

A note on the bisect scripts: if you re-cythonize some code you need to backup the original file before doing so and then restore it after the build. Otherwise bisect can't switch to the next commit due to changed in a filke under source control.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 11, 2012

Member

I think this is ready to merge, unless you still want to fix the function names in error messages.

Member

rgommers commented Nov 11, 2012

I think this is ready to merge, unless you still want to fix the function names in error messages.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 11, 2012

Member

Ok, made the cast warning always be printed (outside the usual system since it's not really a floating point accuracy warning), and got rid of the '?'.

Member

pv commented Nov 11, 2012

Ok, made the cast warning always be printed (outside the usual system since it's not really a floating point accuracy warning), and got rid of the '?'.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 11, 2012

Member

Still works fine. Always showing legacy warning is a good idea.

Member

rgommers commented Nov 11, 2012

Still works fine. Always showing legacy warning is a good idea.

pv added a commit that referenced this pull request Nov 11, 2012

Merge pull request #334 from pv/special-cleanup-errors
MAINT: special: refactor error handling

Refactor error handling in scipy.special. The behavior is as follows:

- Internal reorganization: Special function accuracy warnings are
  emitted with new flexible sf_error() function, instead of mtherr().

- Casting: unsafe casts produce NaNs if they are not exact.

- Unsafe cast loops (apart from long->int which is more or less
  necessary on 64-bit) are not automatically generated.
  For instance, passing doubles to integer arguments raises
  an error now.

- Legacy: however, all old functions that truncated floats to
  integers retain their previous behavior (= manual special casing).
  If double -> integer truncation occurs, a warning is raised.

- Floating point exception flags are now caught and handled with
  the same mechanism as the rest of the special function warnings.
  Previously, these were caught by the usual ufunc machinery.
  This is more a nuisance than help ---
  the special function implementations do not set overflow/invalid
  flags when returning infs or nans, so the fp exception status
  cannot be relied on. So I think it is better to make them
  disabled by default, and allow the user to control them
  the same way as the rest of the provisional accuracy warnings.

@pv pv merged commit 9036251 into scipy:master Nov 11, 2012

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Nov 11, 2012

Member

Ok, merged. Thanks for your help here.

I think the special function infrastructure is now more or less OK, the next (huge) task would be to make sure the implementations actually are sufficiently accurate, and fix them whenever not.

Member

pv commented Nov 11, 2012

Ok, merged. Thanks for your help here.

I think the special function infrastructure is now more or less OK, the next (huge) task would be to make sure the implementations actually are sufficiently accurate, and fix them whenever not.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Nov 12, 2012

Member

Thank you for fixing it up! 0.12 will be a good release for the special module.

Member

rgommers commented Nov 12, 2012

Thank you for fixing it up! 0.12 will be a good release for the special module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment