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

Bug: adds support for unary plus #19297

Merged
merged 12 commits into from
Feb 8, 2018
Merged

Conversation

deniederhut
Copy link
Contributor

@deniederhut deniederhut commented Jan 18, 2018

Adds missing unary plus operator from #16073. Adds typechecking behavior to unary neg, along with tests for both.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can you add a similar set of tests for Series?

@@ -273,6 +273,7 @@ Build Changes
Other API Changes
^^^^^^^^^^^^^^^^^

- Unary ``+`` operator now permitted for ``Series`` and ``DataFrame`` (:issue:`16073`)
Copy link
Contributor

Choose a reason for hiding this comment

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

move to Other Enhancements, add the word numeric operations at the end.


def __pos__(self):
values = _values_from_object(self)
if values.dtype == np.bool_:
Copy link
Contributor

Choose a reason for hiding this comment

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

is_bool_dtype (and can you change in __neg__ as well

@@ -389,12 +389,42 @@ def test_logical_with_nas(self):
assert_series_equal(result, expected)

def test_neg(self):
# what to do?
assert_frame_equal(-self.frame, -1 * self.frame)
numeric = pd.DataFrame({
Copy link
Contributor

Choose a reason for hiding this comment

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

can you parameterize this


def test_invert(self):
assert_frame_equal(-(self.frame < 0), ~(self.frame < 0))

def test_pos(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

same

assert_frame_equal(+numeric, +1 * numeric)
assert_frame_equal(+boolean, (+1 * boolean).astype(bool))
assert_series_equal(+timedelta, pd.to_timedelta(+1 * timedelta))
with pytest.raises(TypeError):
Copy link
Contributor

Choose a reason for hiding this comment

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

move the TypeError check (e.g. passing in invalid dtypes) to a separate test, and parameterize with all the invalid dtypes.

@jreback jreback added Enhancement Dtype Conversions Unexpected or buggy dtype conversions Numeric Operations Arithmetic, Comparison, and Logical operations labels Jan 18, 2018
@deniederhut
Copy link
Contributor Author

From the CI logs, it looks like the new operator trips some of the computational tests for other dtypes. I can clean these up, but would like to ask for some guidance on how we would like to handle operations on the timeperiod dtypes.

@pep8speaks
Copy link

pep8speaks commented Jan 22, 2018

Hello @deniederhut! Thanks for updating the PR.

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

Comment last updated on February 08, 2018 at 01:18 Hours UTC

@jreback
Copy link
Contributor

jreback commented Jan 22, 2018

@deniederhut

@jbrockmendel has been working a lot on the ops recently. what cases are in question? can you show (alternativley, you can xfail them and then point them out in your diff).

@deniederhut
Copy link
Contributor Author

TestSeriesPeriod.test_ops_series_period and TestFramePeriod.test_ops_frame_period both had unary negatives which are currently not allowed for TimePeriod objects, such as:

tm.assert_series_equal(s - p, -exp)

Do we want to allow these?

TestEvalPythonPython.test_frame_pos and TestEvalPythonPython.test_series_pos are both testing to ensure that a unary plus always raises, even with numeric data. My assumption is that these would be okay to remove, since they are testing for the nonexistence of the feature this PR implements.

@deniederhut
Copy link
Contributor Author

@jreback would it be better to defer the decision about TimePeriods, and xfail the problematic tests for now?

@jreback
Copy link
Contributor

jreback commented Jan 27, 2018

yes, ok with fixing Timedelta / periods after. pls put in the appopriate xfails. and rebase.

@deniederhut deniederhut force-pushed the bug/unary-plus branch 2 times, most recently from 7145fe4 to 3d0844a Compare February 2, 2018 11:54
@deniederhut
Copy link
Contributor Author

@jreback It's not clear to me why the Travis build is failing -- there are some warnings about IPython versions and SparseBlocks, but no failed tests. Is there something obvious that I am missing?

@jreback
Copy link
Contributor

jreback commented Feb 5, 2018

you are failing linting

see the very end

@deniederhut
Copy link
Contributor Author

🤦‍♀️ yup there it is. Thanks!

@codecov
Copy link

codecov bot commented Feb 5, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19297      +/-   ##
==========================================
+ Coverage   91.62%   91.62%   +<.01%     
==========================================
  Files         150      150              
  Lines       48773    48790      +17     
==========================================
+ Hits        44687    44704      +17     
  Misses       4086     4086
Flag Coverage Δ
#multiple 89.99% <100%> (ø) ⬆️
#single 41.73% <9.09%> (-0.02%) ⬇️
Impacted Files Coverage Δ
pandas/core/generic.py 95.93% <100%> (+0.01%) ⬆️
pandas/core/indexes/category.py 97.26% <0%> (-0.26%) ⬇️
pandas/core/reshape/merge.py 94.25% <0%> (+0.03%) ⬆️
pandas/core/arrays/categorical.py 94.87% <0%> (+0.13%) ⬆️

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 affb5d9...2b8072e. Read the comment docs.

@@ -111,6 +111,8 @@ def test_df_radd_str(self):

class TestPeriodFrameArithmetic(object):

@pytest.mark.xfail(reason="Unary negative is currently not permitted "
Copy link
Contributor

Choose a reason for hiding this comment

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

@deniederhut so period - period is now going to raise? this is #13077.

I am not actually sure what we should do here. I think we can re-enable these though, at least for now.

(you can use is_period_arraylike to detect this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is marked xfail because of the -exp in this assertion tm.assert_frame_equal(df - p, -exp), not the initial subtraction. Happy to allow it and unmark the tests.

Should the expected return values in the tests be scalars? I think period - period currently returns an integer.

Copy link
Contributor

Choose a reason for hiding this comment

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

i c
the return value should be integers

pd.DataFrame({'a': pd.Series(pd.to_timedelta([1, -1]))}))
])
def test_neg_numeric(self, df, expected):
assert_frame_equal(-df, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jbrockmendel I think on your list is to rationalize locations of arithmetic tests between test_arithmetic and test_operations

@jreback jreback added this to the 0.23.0 milestone Feb 8, 2018
@jreback jreback merged commit b835127 into pandas-dev:master Feb 8, 2018
@jreback
Copy link
Contributor

jreback commented Feb 8, 2018

thanks @deniederhut nice patch! sometimes little issues can morph, nice job!

@deniederhut
Copy link
Contributor Author

Thanks so much for your help @jreback !

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dtype Conversions Unexpected or buggy dtype conversions Enhancement Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unary plus missing for Series and DataFrame
3 participants