-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
BUG: User-facing AssertionError with add column to SparseDataFrame #25503
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25503 +/- ##
==========================================
- Coverage 91.75% 91.75% -0.01%
==========================================
Files 173 173
Lines 52955 52957 +2
==========================================
Hits 48589 48589
- Misses 4366 4368 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25503 +/- ##
==========================================
- Coverage 91.89% 91.89% -0.01%
==========================================
Files 175 175
Lines 52491 52493 +2
==========================================
- Hits 48238 48236 -2
- Misses 4253 4257 +4
Continue to review full report at Codecov.
|
Good start! Definitely will need to add a test for this. |
Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-04-12 12:21:51 UTC |
one of the changes will be hitting this test (updated to catch error type in #25483) pandas/pandas/tests/sparse/frame/test_frame.py Lines 553 to 555 in b99f8bc
|
msg = 'Length of values does not match length of index' | ||
with pytest.raises(ValueError, match=msg): | ||
sp['foo'] = np.random.randn(N-1) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may not need an additional test (#25503 (comment))
the code sample in #25484 replicates the setup of the existing test. so maybe able to alter existing test to hit both changes you've made to pandas\core\sparse\frame.py in _sanitize_column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, i see! thanks! @simonjayhawkins
can you merge master and resolve conflicts. ping on green. |
23b33df
to
4a9f225
Compare
can you merge master & add a whatsnew note for this. ping on green. |
6b5f1d7
to
adf847a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc-change, otherwise lgtm. pls merge master. ping on green.
doc/source/whatsnew/v0.25.0.rst
Outdated
@@ -227,7 +227,7 @@ Performance Improvements | |||
|
|||
Bug Fixes | |||
~~~~~~~~~ | |||
|
|||
- Bug in `SparseDataFrame` when adding a column in which the length of values does not match length of index, ``AssertionError`` is raised instead of raising ``ValueError`` (:issue:`25484`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make a Sparse sub-section and move this there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, sparse section is already there, moved! thanks @jreback
363aabb
to
2fca2ce
Compare
@jreback i merged the master and made the doc change, pls take a look, thanks |
2fca2ce
to
3749db6
Compare
@jreback i just merged the master and resolved the conflict, pls take a look if you have some time. thanks! |
thanks @charlesdong1991 |
git diff upstream/master -u -- "*.py" | flake8 --diff