Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Floor and ceil methods during pandas.eval which are provided by numexpr #24355

Merged
merged 17 commits into from
Dec 30, 2018

Conversation

anjsudh
Copy link
Contributor

@anjsudh anjsudh commented Dec 19, 2018

@pep8speaks
Copy link

pep8speaks commented Dec 19, 2018

Hello @anjsudh! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 25, 2018 at 18:06 Hours UTC

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests too :)

@@ -372,6 +372,7 @@ Other Enhancements
- :meth:`DataFrame.to_stata` and :class:`pandas.io.stata.StataWriter117` can write mixed sting columns to Stata strl format (:issue:`23633`)
- :meth:`DataFrame.between_time` and :meth:`DataFrame.at_time` have gained the an ``axis`` parameter (:issue:`8839`)
- :class:`IntervalIndex` has gained the :attr:`~IntervalIndex.is_overlapping` attribute to indicate if the ``IntervalIndex`` contains any overlapping intervals (:issue:`23309`)
- :meth:`eval()` now supports use of `floor` and `ceil` in the expression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the issue number here. This will also work in DataFrame.query, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this underneath this note:

- Added log10 to the list of supported functions in :meth:`DataFrame.eval` (:issue:`24139`)

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #24355 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24355   +/-   ##
=======================================
  Coverage   92.29%   92.29%           
=======================================
  Files         162      162           
  Lines       51808    51808           
=======================================
  Hits        47817    47817           
  Misses       3991     3991
Flag Coverage Δ
#multiple 90.7% <ø> (ø) ⬆️
#single 42.99% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/computation/ops.py 95.6% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14c33b0...6abf412. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #24355 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24355      +/-   ##
==========================================
+ Coverage   92.31%   92.31%   +<.01%     
==========================================
  Files         166      166              
  Lines       52412    52416       +4     
==========================================
+ Hits        48382    48386       +4     
  Misses       4030     4030
Flag Coverage Δ
#multiple 90.73% <100%> (ø) ⬆️
#single 43.06% <66.66%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/computation/check.py 92.3% <100%> (+1.39%) ⬆️
pandas/core/computation/ops.py 95.63% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ebfd8a...3ccb40c. Read the comment docs.

@WillAyd WillAyd changed the title closes #24353 Floor and ceil methods during pandas.eval which are provided by numexpr Dec 19, 2018
@@ -372,6 +372,7 @@ Other Enhancements
- :meth:`DataFrame.to_stata` and :class:`pandas.io.stata.StataWriter117` can write mixed sting columns to Stata strl format (:issue:`23633`)
- :meth:`DataFrame.between_time` and :meth:`DataFrame.at_time` have gained the an ``axis`` parameter (:issue:`8839`)
- :class:`IntervalIndex` has gained the :attr:`~IntervalIndex.is_overlapping` attribute to indicate if the ``IntervalIndex`` contains any overlapping intervals (:issue:`23309`)
- :meth:`eval()` now supports use of `floor` and `ceil` in the expression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this underneath this note:

- Added log10 to the list of supported functions in :meth:`DataFrame.eval` (:issue:`24139`)

@jreback jreback added the Numeric Operations Arithmetic, Comparison, and Logical operations label Dec 19, 2018
pandas/core/computation/ops.py Outdated Show resolved Hide resolved
pandas/tests/computation/test_eval.py Outdated Show resolved Hide resolved

try:
import numexpr as ne
ver = LooseVersion(ne.__version__)
_NUMEXPR_INSTALLED = ver >= LooseVersion(_MIN_NUMEXPR_VERSION)
_ne_version_under_2_6_9 = ver < LooseVersion('2.6.9')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to add this here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather export _NUMEXPR_VERSION as well

@@ -20,6 +20,7 @@
_TEST_MODE = None
_TEST_RESULT = None
_USE_NUMEXPR = _NUMEXPR_INSTALLED
_ne_version_under2_6_9 = _ne_version_under_2_6_9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to add this here

@@ -23,8 +25,10 @@

_unary_math_ops = ('sin', 'cos', 'exp', 'log', 'expm1', 'log1p',
'sqrt', 'sinh', 'cosh', 'tanh', 'arcsin', 'arccos',
'arctan', 'arccosh', 'arcsinh', 'arctanh', 'abs', 'log10')
'arctan', 'arccosh', 'arcsinh', 'arctanh', 'abs', 'log10',
'floor', 'ceil')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just do this

_unary_math_ops = [...]
if _NUMEXPR_INSTALLED and _NUMEXPR_VERSION >= LooseVersion('2.6.9'):
    _unary_math_ops += ['floor', 'ceil']

pandas/tests/computation/test_eval.py Outdated Show resolved Hide resolved
pandas/tests/computation/test_eval.py Outdated Show resolved Hide resolved
pandas/tests/computation/test_eval.py Outdated Show resolved Hide resolved
@anjsudh
Copy link
Contributor Author

anjsudh commented Dec 25, 2018

Getting an error: "err: pandas should not import: numexpr". I haven't imported numexpr in my diff. The error resolves when I don't import _NUMEXPR_INSTALLED and _NUMEXPR_VERSION. Any ideas on how to go about the version check without the import?

Also, is there a reason for us to check that numexpr is not imported? The import is anyways in a try catch block

Any inputs would help here?

pandas/core/computation/ops.py Outdated Show resolved Hide resolved
@@ -31,6 +32,8 @@
assert_numpy_array_equal, assert_series_equal,
assert_produces_warning)
from pandas.compat import PY3, reduce
from pandas.core.computation.check import _NUMEXPR_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add this on to line 20 above

@jreback jreback added this to the 0.24.0 milestone Dec 25, 2018
@jreback
Copy link
Contributor

jreback commented Dec 25, 2018

lgtm. ping on green.

@jreback
Copy link
Contributor

jreback commented Dec 25, 2018

@@ -14,6 +15,7 @@
import pandas as pd
from pandas.core.base import StringMixin
import pandas.core.common as com
from pandas.core.computation.check import _NUMEXPR_INSTALLED, _NUMEXPR_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anjsudh ok, the problem is we can't import here (we have a check in code_checks to avoid importing the optional dependencies until they are needed).

So. I think just add floor & ceil to the _math_ops (always) and better to do a run-time check if this op exists in ne (not exactly sure how to do that).

@jreback
Copy link
Contributor

jreback commented Dec 28, 2018

can you merge master and update

@anjsudh anjsudh force-pushed the master branch 3 times, most recently from de48cc2 to bf64953 Compare December 30, 2018 09:35
@anjsudh
Copy link
Contributor Author

anjsudh commented Dec 30, 2018

@jreback Hope you can have a look?

@jreback jreback merged commit 2d1d697 into pandas-dev:master Dec 30, 2018
@jreback
Copy link
Contributor

jreback commented Dec 30, 2018

thanks @anjsudh

yeah this was a bit tricky because of our delayed import of numexpr :>

thoo added a commit to thoo/pandas that referenced this pull request Dec 30, 2018
* upstream/master:
  DOC: Fixing broken references in the docs (pandas-dev#24497)
  DOC: Splitting api.rst in several files (pandas-dev#24462)
  Fix misdescription in escapechar (pandas-dev#24490)
  Floor and ceil methods during pandas.eval which are provided by numexpr (pandas-dev#24355)
  BUG: Pandas any() returning false with true values present (GH pandas-dev#23070) (pandas-dev#24434)
  Misc separable pieces of pandas-dev#24024 (pandas-dev#24488)
  use capsys.readouterr() as named tuple (pandas-dev#24489)
  REF/TST: replace capture_stderr with pytest capsys fixture (pandas-dev#24496)
  TST- Fixing issue with test_parquet test unexpectedly passing (pandas-dev#24480)
  DOC: Doc build for a single doc made much faster, and clean up (pandas-dev#24428)
  BUG: Fix+test timezone-preservation in DTA.repeat (pandas-dev#24483)
  Implement reductions from pandas-dev#24024 (pandas-dev#24484)
thoo added a commit to thoo/pandas that referenced this pull request Dec 30, 2018
…strings

* upstream/master:
  TST: Skip db tests unless explicitly specified in -m pattern (pandas-dev#24492)
  Mix EA into DTA/TDA; part of 24024 (pandas-dev#24502)
  DOC: Fix building of a single API document (pandas-dev#24506)
  DOC: Fixing broken references in the docs (pandas-dev#24497)
  DOC: Splitting api.rst in several files (pandas-dev#24462)
  Fix misdescription in escapechar (pandas-dev#24490)
  Floor and ceil methods during pandas.eval which are provided by numexpr (pandas-dev#24355)
  BUG: Pandas any() returning false with true values present (GH pandas-dev#23070) (pandas-dev#24434)
  Misc separable pieces of pandas-dev#24024 (pandas-dev#24488)
  use capsys.readouterr() as named tuple (pandas-dev#24489)
  REF/TST: replace capture_stderr with pytest capsys fixture (pandas-dev#24496)
  TST- Fixing issue with test_parquet test unexpectedly passing (pandas-dev#24480)
  DOC: Doc build for a single doc made much faster, and clean up (pandas-dev#24428)
  BUG: Fix+test timezone-preservation in DTA.repeat (pandas-dev#24483)
  Implement reductions from pandas-dev#24024 (pandas-dev#24484)
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need support for floor and ceil methods during pandas.eval which are provided by numexpr
4 participants