FIX-#3619: Fix ValueError when doing loc on empty dataframe #3631
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
Codecov Report
@@ Coverage Diff @@
## master #3631 +/- ##
===========================================
- Coverage 85.32% 36.90% -48.43%
===========================================
Files 194 181 -13
Lines 16194 15589 -605
===========================================
- Hits 13818 5753 -8065
- Misses 2376 9836 +7460
Continue to review full report at Codecov.
|
|
|
|
|
This pull request fixes 7 alerts when merging 66ae3f9b2248fb8fac3d1ceea2f2539019b30b52 into 648b6a0 - view on LGTM.com fixed alerts:
|
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
naren-ponder
force-pushed
the
empty_loc
branch
from
83ef26e to
3724694
Compare
Signed-off-by: Naren Krishna <naren@ponder.io>
dchigarev
reviewed
Nov 12, 2021
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
Signed-off-by: Naren Krishna <naren@ponder.io>
|
do the |
|
@dchigarev Can you re-run the TeamCity Dask test? I believe it's a flake. |
done, now it's a green |
Signed-off-by: Naren Krishna <naren@ponder.io>
|
@dchigarev Added tests for |
BTW, the actual problem of why the reported bug occurs is the following way of computing a column lookup for modin/modin/pandas/indexing.py Lines 698 to 701 in 4121358 Pandas backend is failing to process [] instead of a slice(None) since for non-full-axis grabs it tries to do some partitioning stuff under an empty frame and fails.
It's actually pretty lucky, that the scenario from reproducer works well for the modin/modin/pandas/indexing.py Lines 817 to 826 in 4121358 So, actually, the reason why
I'm actually working on aligning these inconsistencies in #3520 and #3513, but since they aren't merged, this all looks kinda unsafe. We actually don't know how other backends would behave on an attempt to index an empty frame since there is no essential implementation for an empty core modin frame. Maybe we should bring some safety and consistency in this PR by defaulting to pandas for both: Sorry, for probably being too picky for a small-change PR, I would also be okay with the "if it works, don't touch it" approach 😄 |
I was wondering about this myself, thanks for the explanation! Let me make that change -- I think it would be better stability-wise :) |
Signed-off-by: Naren Krishna <naren@ponder.io>
dchigarev
reviewed
Nov 15, 2021
Signed-off-by: Naren Krishna <naren@ponder.io>
dchigarev
approved these changes
Nov 15, 2021
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.
Signed-off-by: Naren Krishna naren@ponder.io
What do these changes do?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -slocon empty dataframe #3619docs/developer/architecture.rstis up-to-date