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

REF: Simplify quantile, remove reduction from BlockManager #24597

Merged
merged 8 commits into from
Jan 3, 2019

Conversation

jbrockmendel
Copy link
Member

BlockManager.reduction is only ever called for quantile. Might as well remove the layer of indirection so we can simplify reduction (now renamed quantile). Most of the simplification comes in Block.quantile, since we can avoid passing around things we don't need.

Two nested functions currently defined inside Block.quantile are moved outside the closure so I don't have to double-check the namespace every time I look at them. Not sure if they belong somewhere else.

@@ -3365,3 +3324,37 @@ def _putmask_preserve(nv, n):
v = v.astype(dtype)

return _putmask_preserve(v, n)


# TODO: belongs elsewhere?
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 nanops

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.

will look in detail in a bit

@jbrockmendel
Copy link
Member Author

just moved + pushed. hopefully I'll get the docstrings in place before you have to remind me.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #24597 into master will decrease coverage by 49.34%.
The diff coverage is 9.8%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24597       +/-   ##
===========================================
- Coverage   92.38%   43.04%   -49.35%     
===========================================
  Files         166      166               
  Lines       52478    52481        +3     
===========================================
- Hits        48483    22591    -25892     
- Misses       3995    29890    +25895
Flag Coverage Δ
#multiple ?
#single 43.04% <9.8%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/internals/blocks.py 52.82% <10%> (-41.68%) ⬇️
pandas/core/nanops.py 31.27% <10%> (-63.64%) ⬇️
pandas/core/internals/managers.py 66.22% <9.09%> (-29.71%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/core/categorical.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-95.46%) ⬇️
... and 127 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 62506ca...379fdde. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5ba4337). Click here to learn what that means.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #24597   +/-   ##
=========================================
  Coverage          ?   92.38%           
=========================================
  Files             ?      166           
  Lines             ?    52488           
  Branches          ?        0           
=========================================
  Hits              ?    48491           
  Misses            ?     3997           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.81% <94.11%> (?)
#single 43.05% <9.8%> (?)
Impacted Files Coverage Δ
pandas/core/internals/managers.py 95.96% <100%> (ø)
pandas/core/internals/blocks.py 94.54% <100%> (ø)
pandas/core/nanops.py 94.54% <85%> (ø)

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 5ba4337...11af1dd. Read the comment docs.



def _nanpercentile1D(values, mask, q, na_value, interpolation):
# mask is Union[ExtensionArray, ndarray]
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 add doc-strings

Copy link
Member Author

Choose a reason for hiding this comment

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

just pushed with docstrings. We're going to simplify the tar out of these methods if/when #24600 gets fixed.

@jreback jreback added the Clean label Jan 3, 2019
@jbrockmendel
Copy link
Member Author

one more quantile PR in the pipeline after this

"""

if consolidate:
self._consolidate_inplace()

def get_axe(block, qs, axes):
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this adds anything to make it a function

axe, block = getattr(b, f)(axis=axis, axes=self.axes, **kwargs)
block = b.quantile(axis=axis, qs=qs, interpolation=interpolation)

axe = get_axe(b, qs, axes=self.axes)
Copy link
Contributor

Choose a reason for hiding this comment

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

also doesn't need / take the bock arg

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 uses the block arg

Copy link
Contributor

Choose a reason for hiding this comment

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

no i mean get_axe doens't

Copy link
Member Author

Choose a reason for hiding this comment

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

(which is why it is a function instead of just done once outside the loop).

I'd rather keep it as a function than in-line it, but not a deal-breaker. There is another PR after this that will be ripping out a bunch of code regardless.

Copy link
Member Author

Choose a reason for hiding this comment

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

no i mean get_axe doens't

line 435 inside get_axe reads elif block.ndim == 1:

Copy link
Contributor

Choose a reason for hiding this comment

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

grr, ok, i c now

pandas/core/nanops.py Outdated Show resolved Hide resolved
@jreback jreback added the Numeric Operations Arithmetic, Comparison, and Logical operations label Jan 3, 2019
@jreback jreback added this to the 0.24.0 milestone Jan 3, 2019
@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

ping on green. followup to clean internals very welcome!

@jreback jreback merged commit 1d7623f into pandas-dev:master Jan 3, 2019
@jreback
Copy link
Contributor

jreback commented Jan 3, 2019

thanks @jbrockmendel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants