REGR: revert "CLN: _consolidate_inplace less" / fix regression in fillna() #34407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 760ba37.
|
do the asvs show any of the perf issues mentioned in #34389? |
|
The benchmark server isn't running at the moment, see #34389 (comment) |
|
i was hoping you'd be able to run them locally. (as mentioned in the other thread, im wrestling with hardware issues ATM) |
|
Sorry, I currently don't have time to run the full ASV suite. |
jreback
added
Clean
Internals
|
ive gotten asv working locally, will do a run on #34389 if were OK on holding off on merging this |
|
As I also said in #34389, running asv is not necessarily sufficient, as it not sure that the functions that are tested are actually covered (it's not that our benchmark suite is 100% covering all cases, and even then, they don't necessarily include cases that start with non-consolidated data). |
|
Or, for some of them it might be sufficient to simply check in the code if it would matter or not. |
Let's un-revert that here and whittle down the places that merit double-checking |
|
I'm not seeing any consistent pattern in asv results |
Done |
|
@jbrockmendel what's the status here? (I re-reverted the ones discussed above) Do we continue with this PR reverting those, or do you have time to check those cases? |
Thanks for following up. I've gotten my hardware issues resolved, will run a round of asvs on this. |
|
As mentioned before (#34407 (comment)) and discussed elsewhere, I am not sure that asv will give any useful information for those changes. |
Neither am I. But if they do show something, that will help us whittle down the places that need more manual attention. |
|
But what I want to say is: even if they don't show anything, each case still needs manual attention, as the asv's (AFAIK) don't include non-consolidated data. For example, a small timing for Do we care about this difference, I am not fully sure (that's to be discussed then). But it clearly has an impact. |
|
But what I want to say is: I haven't had my caffeine yet, and running asvs now doesn't preclude doing %timeits later |
|
Full asv run results below. A lot of these look like they may be the result of not being rebased on master, so I'm going to do that and re-run. |
3 tasks
|
@jorisvandenbossche is this still active? |
|
i am not sure it makes sense to push this w/o a clear patch backed by tests. As i am not sure what excatly this is fixing. |
|
moving this off 1.1.4 as not really clear metrics on how to evaluate this. |
|
not sure what the point of the PR Is any longer. |
There are still open regressions related to this (#36495). |
maybe so but we still don't have any testing. so -1 on including this on 1.1.5 at this point. |
jorisvandenbossche
changed the title
I added a test for the regression reported in #36495, which is fixed by this |
| ) | ||
| df_nonconsol = df.pivot("i1", "i2") | ||
| result = df_nonconsol.fillna(0) | ||
| assert result.isna().sum().sum() == 0 |
There was a problem hiding this comment.
there's a test in #36668 if you wanted a more explicit assertion
3 tasks
great. there are four consolidations this adds. is any one in particular responsible for fixing this bug? |
|
I suppose it is the consolidation in I could do a PR with specifically only that change, but as mentioned above (#34407 (comment)), I think it can't hurt to change the other cases as well. Since the original PR #34389, we now already reverted parts of that change in 3 other PRs that were fixing regressions, and this is a 4th. |
|
thanks @jorisvandenbossche |
|
@meeseeksdev backport 1.1.x |
This comment has been minimized.
This comment has been minimized.
simonjayhawkins
pushed a commit
to simonjayhawkins/pandas
that referenced
this pull request
Nov 27, 2020
…solidate_inplace less" / fix regression in fillna()
simonjayhawkins
added a commit
that referenced
this pull request
Nov 27, 2020
…nplace less" / fix regression in fillna() (#38115) Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
|
AFAICT the consolidation in quantile is never reached. am i missing something? |
|
The default of the keyword is True, so it should be reached? (since we never specify this keyword anywhere at the moment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #34389
Closes #36495