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

clarify redirection in ops #19346

Merged
merged 4 commits into from
Jan 25, 2018
Merged

clarify redirection in ops #19346

merged 4 commits into from
Jan 25, 2018

Conversation

jbrockmendel
Copy link
Member

core.ops involves a lot of redirection. This decreases some of that redirection (and some redundancy) by implementing functions that show the logic of how some of the args/kwargs are chosen. The goal is to make it so future debugging can be done without going through func.im_func.func_closure[1].cell_contents.func_closure...

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

@codecov
Copy link

codecov bot commented Jan 22, 2018

Codecov Report

Merging #19346 into master will increase coverage by 0.01%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19346      +/-   ##
==========================================
+ Coverage   91.57%   91.58%   +0.01%     
==========================================
  Files         150      150              
  Lines       48702    48711       +9     
==========================================
+ Hits        44597    44611      +14     
+ Misses       4105     4100       -5
Flag Coverage Δ
#multiple 89.95% <98.61%> (+0.01%) ⬆️
#single 41.76% <98.61%> (+0.04%) ⬆️
Impacted Files Coverage Δ
pandas/core/ops.py 95.29% <100%> (+0.34%) ⬆️
pandas/core/sparse/array.py 91.82% <100%> (ø) ⬆️
pandas/core/sparse/series.py 95.3% <100%> (-0.02%) ⬇️
pandas/core/panel.py 96.95% <80%> (+0.12%) ⬆️
pandas/plotting/_converter.py 65.22% <0%> (-1.74%) ⬇️
pandas/util/testing.py 83.83% <0%> (-0.62%) ⬇️
pandas/core/series.py 94.6% <0%> (ø) ⬆️
pandas/core/groupby.py 92.15% <0%> (+0.01%) ⬆️
pandas/core/indexing.py 93.11% <0%> (+0.49%) ⬆️
... and 2 more

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 2952fbd...fe78b73. Read the comment docs.

@jbrockmendel jbrockmendel reopened this Jan 24, 2018
@gfyoung gfyoung added Internals Related to non-user accessible pandas implementation Clean labels Jan 24, 2018
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.

lgtm. some minor style comments. any idea what masker is for, can remove?

equiv = typ + ' ' + op_desc['op'] + ' other'

if typ == 'series':
doc = _flex_doc_SERIES % (op_desc['desc'], op_name, equiv,
Copy link
Contributor

Choose a reason for hiding this comment

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

pass these as .format()

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. While I'm at it I'm going to bring the Panel docstring over here too and put the docstring templates in a dedicated section of the file. Other than that nothing changing in the next push (other than addressing comments).

Parameters
----------
op_name : str
typ : str
Copy link
Contributor

Choose a reason for hiding this comment

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

give allowed values

op_desc['reverse'])
elif typ == 'dataframe':
doc = _flex_doc_FRAME % (op_desc['desc'], op_name, equiv,
op_desc['reverse'])
Copy link
Contributor

Choose a reason for hiding this comment

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

else raise an AssertionError

def _flex_comp_method_FRAME(op, name, str_rep=None, default_axis='columns',
masker=False):
def _flex_comp_method_FRAME(op, name, str_rep=None, default_axis='columns'):
# Masker unused for now
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you removed masker elsewhere? remove the comment

"""
Wrapper function for Series arithmetic operations, to avoid
code duplication.
"""
masker = _gen_eval_kwargs(name).get('masker', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is needed? (masker)?

-------
eval_kwargs : dict


Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

@jreback jreback added this to the 0.23.0 milestone Jan 24, 2018
@jbrockmendel
Copy link
Member Author

any idea what masker is for, can remove?

It's a fill value used for Series comparisons. The only place it is used ATM is in _comp_method_SERIES, where it defaults to False and is passed as True for __ne__.

@jreback
Copy link
Contributor

jreback commented Jan 25, 2018

ping on green. I still have to check that the doc-strings are unchanged from previous though.

@jbrockmendel
Copy link
Member Author

ping

@jreback
Copy link
Contributor

jreback commented Jan 25, 2018

thanks!

@jreback jreback merged commit 86d9af0 into pandas-dev:master Jan 25, 2018
@jbrockmendel jbrockmendel deleted the ops_kwargs branch February 11, 2018 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants