REGR: revert "CLN: _consolidate_inplace less" / fix regression in fillna() #34407
Conversation
This file contains hidden or 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.
Member
| do the asvs show any of the perf issues mentioned in #34389? |
Member Author
| The benchmark server isn't running at the moment, see #34389 (comment) |
Member
| i was hoping you'd be able to run them locally. (as mentioned in the other thread, im wrestling with hardware issues ATM) |
Member Author
| Sorry, I currently don't have time to run the full ASV suite. |
jreback added Clean Internals Member
| ive gotten asv working locally, will do a run on #34389 if were OK on holding off on merging this |
Member Author
| 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). |
Member Author
| Or, for some of them it might be sufficient to simply check in the code if it would matter or not. |
Member
Let's un-revert that here and whittle down the places that merit double-checking |
Member
| I'm not seeing any consistent pattern in asv results |
Member Author
Done |
Member Author
| @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? |
Member
Thanks for following up. I've gotten my hardware issues resolved, will run a round of asvs on this. |
Member Author
| As mentioned before (#34407 (comment)) and discussed elsewhere, I am not sure that asv will give any useful information for those changes. |
Member
Neither am I. But if they do show something, that will help us whittle down the places that need more manual attention. |
Member Author
| 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. |
Member
| But what I want to say is: I haven't had my caffeine yet, and running asvs now doesn't preclude doing %timeits later |
Member
| 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
Member
| @jorisvandenbossche is this still active? |
Member
Contributor
| 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. |
Contributor
| moving this off 1.1.4 as not really clear metrics on how to evaluate this. |
Contributor
| not sure what the point of the PR Is any longer. |
Member Author
There are still open regressions related to this (#36495). |
Contributor
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 Member Author
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 |
Member
There was a problem hiding this comment.
there's a test in #36668 if you wanted a more explicit assertion
3 tasks
Member
great. there are four consolidations this adds. is any one in particular responsible for fixing this bug? |
Member Author
| 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. |
Contributor
| thanks @jorisvandenbossche |
Member
| @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>
Member
| AFAICT the consolidation in quantile is never reached. am i missing something? |
Member Author
| 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