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

Centralize ops kwarg specification #19396

Merged
merged 5 commits into from
Jan 27, 2018
Merged

Conversation

jbrockmendel
Copy link
Member

Follow-up to #19346, making **kwargs more explicit, documenting how they are chosen, moving one method from Panel that belongs in core.ops.

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

"""
Adds the full suite of flex arithmetic methods (``pow``, ``mul``, ``add``)
to the class.

Parameters
----------
flex_arith_method : function
factory for special arithmetic methods, with op string:
f(op, name, str_rep, default_axis=None)
factory for flex arithmetic methods, with op string:
Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure "special" here was a copy/paste mistake. Please correct me if I'm wrong.

@@ -637,15 +632,9 @@ def safe_na_op(lvalues, rvalues):
with np.errstate(all='ignore'):
return na_op(lvalues, rvalues)
except Exception:
if isinstance(rvalues, ABCSeries):
Copy link
Member Author

Choose a reason for hiding this comment

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

isinstance(rvalues, ABCSeries) is always False because before calling safe_na_op we do this same check and if True set rvalues = rvalues.values. If reviewers want to call this out-of-scope I'll remove it and make a separate PR.

@@ -1228,6 +1217,40 @@ def f(self, other, axis=None):
return f


def _flex_method_PANEL(op, name, str_rep=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

Cut/pasted from Panel, with small re-workings to _agg_doc_PANEL.

@@ -1575,7 +1528,8 @@ def f(self, other, axis=0):
'minor_axis': 'columns'})

ops.add_special_arithmetic_methods(Panel, **ops.panel_special_funcs)
Panel._add_aggregate_operations()
ops.add_flex_arithmetic_methods(Panel, ops._flex_method_PANEL,
Copy link
Member Author

Choose a reason for hiding this comment

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

Making this match the usage everywhere else.

@@ -41,13 +41,12 @@
# Wrapper function for Series arithmetic methods


def _arith_method_SPARSE_SERIES(op, name, str_rep=None, default_axis=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

Considering moving this and _arith_method_SPARSE_ARRAY to ops, so that all of the funcs called by ops.add_special* are in ops. (Also there is some duplication that may be removable). Thoughts?

@codecov
Copy link

codecov bot commented Jan 25, 2018

Codecov Report

Merging #19396 into master will increase coverage by <.01%.
The diff coverage is 95.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19396      +/-   ##
==========================================
+ Coverage   91.65%   91.65%   +<.01%     
==========================================
  Files         150      150              
  Lines       48726    48733       +7     
==========================================
+ Hits        44658    44667       +9     
+ Misses       4068     4066       -2
Flag Coverage Δ
#multiple 90.02% <95.5%> (ø) ⬆️
#single 41.76% <55.05%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/sparse/array.py 91.48% <100%> (-0.34%) ⬇️
pandas/core/panel.py 97.3% <100%> (+0.34%) ⬆️
pandas/core/sparse/frame.py 94.8% <100%> (ø) ⬆️
pandas/core/sparse/series.py 95.26% <100%> (-0.04%) ⬇️
pandas/core/ops.py 95.52% <95.18%> (+0.22%) ⬆️
pandas/core/reshape/merge.py 94.21% <0%> (ø) ⬆️
pandas/util/testing.py 83.85% <0%> (+0.01%) ⬆️
pandas/io/parquet.py 72.07% <0%> (+0.51%) ⬆️

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 d3f7d2a...89692f7. Read the comment docs.

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.

looks fine, couple of comments.

# have default axis None, whereas flex methods have default axis 'columns'
subtyp = getattr(cls, '_subtyp', '')
use_numexpr = 'sparse' not in subtyp
# numexpr is available for non-sparse classes
Copy link
Contributor

Choose a reason for hiding this comment

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

comments above the statement


Parameters
----------
other : {construct} or {cls_name}
axis : {{{axis_order}}}
other : DataFrame or Panel
Copy link
Contributor

Choose a reason for hiding this comment

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

is this other correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

It matches the existing docstring

@jreback jreback added the Clean label Jan 26, 2018
@jreback jreback added this to the 0.23.0 milestone Jan 27, 2018
@jreback jreback merged commit 5f6c80b into pandas-dev:master Jan 27, 2018
@jreback
Copy link
Contributor

jreback commented Jan 27, 2018

thanks!

@jbrockmendel jbrockmendel deleted the ops_kwargs2 branch January 31, 2018 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants