BUG: Fix __truediv__ numexpr error
State: closed

Fixes a number of items relating to accelerated arithmetic in frame.

---

`__truediv__` had been set up to use numexpr, but this happened by passingthe division operator as the string. numexpr's evaluate only checked 2 frames up, which meant that it picked up the division setting from`frame.py` and would do floor/integer division when both inputs were integers.You'd only see that issue with a dataframe large enough to trigger numexpr evaluation (>10000 cells)

This adds test cases to `test_expression.py` that [exhibit this failure under thePython2.7 full deps test](https://travis-ci.org/jtratner/pandas/builds/7822069). The testcases only test `Series` and `DataFrame` (though it looks like neither`Series` nor `Panel` use `numexpr`). It doesn't fail under Python 3 because integer division is totally gone.

Now `evaluate`, `_evaluate_standard` and `_evaluate_numexpr` all accept extra keyword arguments, which are passed to `numexpr.evaluate`.

The test case is currently a separate commit that fails. I wasn't sure whether I should have combined it with the bugfix commit or not. Happyto change it if that's more appropriate.


Labels:

Comments:
didn't realized ne could do truediv...good to know

fyi....easy to mod panel to use ne, series should be straightforward as well....

going to add an issue about that

see #3765

FYI, i just pushed this: c278ca63294cb3e42bd3b36d041766cc26b02c76, because of the original failing test

numexpr is only installed on 1 travis build (the 3.2), BUT it is under numpy 1.6.1 which causes weirdness....hmmm

@jreback so why did it fail on 2.7? that test only runs if numexpr is installed. Also, probably a good idea to enable numexpr for one of the 2.x builds, right?

it failed on 2.7, weird (failed for 3.2 for me).....

...just testing enabling numexpr for all builds

@jreback check out the travis ci link: https://travis-ci.org/jtratner/pandas/builds/7822069 . 

(that link is for a different build, obviously). Just for clarity - what needs to happen to incorporate this fix? Or are you saying this still fails in 3.2?

that s your test though (numexpr IS enabled on that build)...look at the print_versions at the very bottom (you have to click it)

I can't believe this never failed before...really weird https://www.travis-ci.org/jreback/pandas/jobs/7824130
fixed now though

@jreback fixed the test to not use div. obviously `test_expressions` wasn't running in python 3.

yep.....numexpr was only recently turned on....since we are now using it practially everythwere going to turn it on

@jreback so yeah, I'm happy to help out. probably can't get to putting this into `Series` and `Panel` today, but hopefully by the end of the week. What's your position on putting in new tests with fixes? Should I have put the test together with the fix / after the fix? Just wondering so I have a sense of how to contribute well to this project.

I usually write the test first, make sure it breaks, then put the fix in
(it could be the same commit or not)...

if I get too many commits or I am 'trying' to fix something...then I just squash them at the end...

e.g. the HDFStore py3k this was like 20 commits...squashed back into 5....had to try a bunch of things!

@jreback is there any way to confirm that numexpr was triggered? I added some print statements for my own testing, but I'm unclear whether `test_expressions.py`, as it stands now, would fail if the actual numexpr call wasn't working. In other words, it would stil be getting the right numerical result, but it wouldn't use the speedup from numexpr.

put in some temporary print statements around the actual call to ne.evaluate (I think I may have had them at one time, but took them out)

FYI...you might be interested in the _long_ discussion about #3393, ne going to get even heavier use going forward

@jreback just to be clear, what I mean is that if you set `pandas.core.expressions` to only use `_evaluate_standard` and `_where_standard`, none of the tests in `test_expressions.py` fail. Similarly, if you add a `ValueError` in the middle of `_evaluate_numexpr` that causes it to always fall back to `_evaluate_standard`, all the tests pass. Any ideas on how to enforce it using `numexpr` (or maybe that would come out with performance tests?) Just wondering if you think this is a problem.

``` python
def _evaluate_numexpr(op, op_str, a, b, raise_on_error = False, **eval_kwargs):
    result = None

    if _can_use_numexpr(op, op_str, a, b, 'evaluate'):
        try:
            a_value, b_value = a, b
            if hasattr(a_value,'values'):
                a_value = a_value.values
            if hasattr(b_value,'values'):
                b_value = b_value.values
            # This causes fallback to _evaluate_standard
            raise ValueError("unknown type object")
            result = ne.evaluate('a_value %s b_value' % op_str, 
                                 local_dict={ 'a_value' : a_value, 
                                              'b_value' : b_value }, 
                                 casting='safe', **eval_kwargs)
        except (ValueError), detail:
            if 'unknown type object' in str(detail):
                pass
        except (Exception), detail:
            if raise_on_error:
                raise TypeError(str(detail))

    if result is None:
        result = _evaluate_standard(op,op_str,a,b,raise_on_error)

    return result
```

@jtratner 

2 different issues here

1) `numexpr` is technically optional, so we _always_ need to be able to fall back if `numexpr` is not installed, also fallback if its not efficient to use it, or the dtypes are wrong
2) tests, there are the `set_use_numexpr` to explicity make it use, this is for testing (in a similar vein, the `set_numexpr_threads` is for perf testing (see the vb_suite)) - `set_use_numexpr` will explicity set/reset `_evaluate` to point to the right ones (which can actually STILL fallback if there is an exception in numexpr, always have to guard against that)

answer your question?

Thanks for digging into this with me - I appreciate you taking the time to
explain!

I get what you're saying and it makes sense. However, test_expressions
doesn't even get run unless `numexpr` is installed, so you can assert
within that module that everything ought to be working correctly.  What I'm
wondering is how you test for the specific case where `numexpr` is
installed but panda's way of calling it fails.  `_evalute_numexpr` turns
everything into a `TypeError` which gets caught and handled by
`_arith_method`, so I'm not sure how you check for that to make sure pandas
is integrating correctly with numexpr.

On Wed, Jun 5, 2013 at 9:02 PM, jreback notifications@github.com wrote:

> @jtratner https://github.com/jtratner
> 
> 2 different issues here
> 
> 1) numexpr is technically optional, so we _always_ need to be able to
> fall back if numexpr is not installed, also fallback if its not efficient
> to use it, or the dtypes are wrong
> 2) tests, there are the set_use_numexpr to explicity make it use, this is
> for testing (in a similar vein, the set_numexpr_threads is for perf
> testing (see the vb_suite)) - set_use_numexpr will explicity set/reset
> _evaluate to point to the right ones (which can actually STILL fallback
> if there is an exception in numexpr, always have to guard against that)
> 
> answer your question?
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3764#issuecomment-19019172
> .

numexpr rarely raises itself (mainly on a type issue, which we catch), or the catchall (which Exception), I think if some other kind of error, don't remember. We only bail on the catchall

otherwise it is computed as normal (which can itself raise an exception, but only a weird case I think)

so its not tested per se that calling from a data frame works (and uses numexpr when it is installed). 

I suppose you could add some functions to `expressions` which would set/unset a module level flag for the last computation done (and maybe status info, would have to sort of 'record' which path actually is taken)

that way you could do something like this:

```
from pandas.core import expressions as e
e.set_test_mode(T)
result = big_df + 1
test_result = e.get_test_results()
```

and `get_test_mode` can return (and invalid the module level data which `set_test_mode` sets)

and `test_result` could be say:

```
test_result = dict(computation = ...., result = ...., path = ....)
```

whatever

Sounds like it's not really worth the time/overhead to check though.

what do u mean?

Well, as in: would it slow things down much to check for a global variable each time? do you think it's actually worth checking for?

this was just for testing

you would have a single check inside evaluate that would set the test result

you could actually set the status each time but you don't want to store the result at all because of he memory refs

easy enough to store the passed string and and the status

I want to leave the test to check that numexpr is actually _used_ to be included with the #3765 , since that will probably turn into a more general check that has to happen for all expressions tests. Anything else I need to do before this can be merged? (Hopefully this gets into 0.11.1, since it reflects a numerical bug, rather than performance bug).

Update on this: everything passes now and I added all the standard arithmetic operators to the test cases. This exposed a regression where `__mod__` was given a `str_rep` of `*` instead of `%`. Unfortunately, I kept getting floating point exceptions when I ran the tests for `__mod__` accelerated by `numexpr`, so I just removed `str_rep` so it will use numpy's `operator.mod` instead. (I think this was added on 5/14, so doesn't need to go in `RELEASE.rst`)

Accelerating `//` caused an inconsistency too (though I don't think it was actually set up) where, when dividing by zero, numpy returns `0.0000` whereas `numexpr` evaluates to `inf`. Not sure which is correct, so it's also not included.

In summary, this pull request adds test cases for `expressions.py` for all major operators and tests `Series` (which isn't currently accelerated) and `Frame` with >10,000 cells. It also fixes `__truediv__` to actually do truedivision when evaluated under `numexpr` (previously was doing integer/floor division) and fixes `__mod__` by removing the incorrect str_rep being passed to `numexpr`.

@jtratner see here: http://pandas.pydata.org/pandas-docs/dev/whatsnew.html (the module/div section). I believe numpy is wrong in returning 0.0000, but numexpr is correct in return `np.inf`; that's what I 'fixed' pandas to do (float division is correct on both numpy/ne for all cases), but for some reason numpy does weird things with ints....so just make its consistent....

nice job on this...

Thanks!

On mod/floordiv - can you show me where you implemented that fix? is that
with the com._fill_zeros?

exactly

I can be reached on my cell 917-971-6387

On Jun 8, 2013, at 10:57 PM, Jeff Tratner notifications@github.com wrote:

> Thanks! 
> 
> On mod/floordiv - can you show me where you implemented that fix? is that 
> with the com._fill_zeros? 
> 
> On Sat, Jun 8, 2013 at 10:50 PM, jreback notifications@github.com wrote: 
> 
> > @jtratner https://github.com/jtratner see here: 
> > http://pandas.pydata.org/pandas-docs/dev/whatsnew.html (the module/div 
> > section). I believe numpy is wrong in returning 0.0000, but numexpr is 
> > correct in return np.inf; that's what I 'fixed' pandas to do (float 
> > division is correct on both numpy/ne for all cases), but for some reason 
> > numpy does weird things with ints....so just make its consistent.... 
> > 
> > nice job on this... 
> > 
> > — 
> > Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/3764#issuecomment-19159796 
> > . 
> > 
> > —
> > Reply to this email directly or view it on GitHub.

I think I figured out the issue...didn't include the explicit requirement to fill zeros on all...will fix soon.