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

CLN: _consolidate_inplace less #34389

Merged
merged 1 commit into from
May 26, 2020

Conversation

jbrockmendel
Copy link
Member

No description provided.

@jreback
Copy link
Contributor

jreback commented May 26, 2020

great. any perf impact? (i know this means running a big asv.....)

@jreback jreback added Clean Internals Related to non-user accessible pandas implementation labels May 26, 2020
@jreback jreback added this to the 1.1 milestone May 26, 2020
@jbrockmendel
Copy link
Member Author

i know this means running a big asv

im dealing with hardware issues that keep me from running this for a while

@jreback
Copy link
Contributor

jreback commented May 26, 2020

i know this means running a big asv

im dealing with hardware issues that keep me from running this for a while

ok happy to merge this now then

@jreback jreback merged commit 760ba37 into pandas-dev:master May 26, 2020
@jbrockmendel jbrockmendel deleted the consolidate-less-1 branch May 27, 2020 02:19
@jorisvandenbossche
Copy link
Member

I don't think this should have been merged without any discussion or performance check?

jorisvandenbossche added a commit that referenced this pull request May 27, 2020
@jreback
Copy link
Contributor

jreback commented May 27, 2020

sure the nightly regression should show this in any event: https://pandas.pydata.org/speed/pandas/#regressions?sort=1&dir=desc

but maybe note updating recently? @TomAugspurger

@TomAugspurger
Copy link
Contributor

TomAugspurger commented May 27, 2020 via email

@jorisvandenbossche
Copy link
Member

the nightly regression should show this in any event

Not necessarily, we don't have benchmarks for all functionality and use cases.

The current BlockManager is optimized to deal with consolidated blocks. At once not consolidating anymore before certain operations, has the potential to cause a slowdown for those operations (where the slowdown could in principle be more than the gain of not consolidating). It might very well be that none of the changes has any impact. But I can't tell that without checking. And I don't think we should do that without checking.
So my proposal is to revert the PR for now, until someone has time to check it.

@jorisvandenbossche
Copy link
Member

(and note that this is not in conflict with what I am saying on the mailing list. It's my opinion that we should optimize the case of "many 1D blocks" in the internals, but that doesn't mean that right now "many 2D blocks" is optimized).

@jreback
Copy link
Contributor

jreback commented May 27, 2020

its fine to revert if you want @jbrockmendel can you put up a PR. but going to continue optimizing generally anyhow.

@jorisvandenbossche
Copy link
Member

I already made the branch before (using github's "revert" button), so -> #34407

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.

4 participants