Skip to content

BUG: Index.str.partition not nan-safe (#23558) #23618

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Nov 18, 2018
Merged

BUG: Index.str.partition not nan-safe (#23558) #23618

merged 11 commits into from
Nov 18, 2018

Conversation

meiermark
Copy link
Contributor

@meiermark meiermark commented Nov 10, 2018 •
edited by jreback
Loading

@pep8speaks
Copy link

pep8speaks commented Nov 10, 2018 •
edited
Loading

Hello @meiermark! Thanks for updating the PR.

Comment last updated on November 18, 2018 at 01:05 Hours UTC

@codecov
Copy link

codecov bot commented Nov 10, 2018 •
edited
Loading

Codecov Report

Merging #23618 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #23618   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files         161      161           
  Lines       51408    51408           
=======================================
  Hits        47416    47416           
  Misses       3992     3992
Flag Coverage Δ
#multiple 90.62% <ø> (ø) ⬆️
#single 42.3% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1250500...4a20094. Read the comment docs.

@gfyoung gfyoung added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Strings String extension data type and string data Indexing Related to indexing on series/frames, not to indexes themselves Bug labels Nov 11, 2018
gfyoung
gfyoung reviewed Nov 11, 2018
View reviewed changes
@meiermark meiermark force-pushed the index_str_partition_not_nan_safe branch from deab820 to 10552b5 Compare November 11, 2018 01:42
@jreback jreback added this to the 0.24.0 milestone Nov 11, 2018
jreback
jreback requested changes Nov 11, 2018
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a whatsnew note (bug fixes in string section). lgtm. otherwise

@jreback
Copy link
Contributor

jreback commented Nov 11, 2018

cc @h-vetinari if you want to have a look.

@meiermark meiermark closed this Nov 11, 2018
@meiermark meiermark deleted the index_str_partition_not_nan_safe branch November 11, 2018 21:57
@meiermark meiermark restored the index_str_partition_not_nan_safe branch November 11, 2018 21:57
@h-vetinari
Copy link
Contributor

@jreback @meiermark

Not sure if this is closed intentionally or permanently. It's a valid proposal to change the output of to_object_array_tuples, but primarily, the problem should be tackled through the MultiIndex constructor, which is the actual problem for .str.partition, see #23578. I've got a solution already as part of #23167, but this is currently blocked by / waiting for #23582.

@meiermark meiermark reopened this Nov 11, 2018
@meiermark
Copy link
Contributor Author

Sorry for the confusion. I destroyed my branch and needed to restore it. Unfortunately, the pull request was closed by this.

@meiermark
Copy link
Contributor Author

meiermark commented Nov 11, 2018 •
edited
Loading

@jreback The whatsnew info was added with the last commit.

jreback
jreback requested changes Nov 11, 2018
View reviewed changes
@h-vetinari
Copy link
Contributor

@meiermark
This should very likely also solve #23677. Could you have a look, and maybe adapt the tests accordingly as well?

@meiermark
Copy link
Contributor Author

@h-vetinari I extended some tests to check #23677. Your assumption was right, it's also resolved.
@jreback can you have a look again? The current state should include the required changes.

jreback
jreback requested changes Nov 16, 2018
View reviewed changes
@jreback
Copy link
Contributor

jreback commented Nov 16, 2018

does this close both issues at the top of the PR? can you annotate a test where they are covered. also add to the whatsnew both issue numbers.

@meiermark
DOC Added resolved issue annotation to tests
bb94c38 
DOC Added whatsnew message for resolved issue pandas-dev#23677
@meiermark
Copy link
Contributor Author

@jreback Yes, this PR closes both issues. Added comments to the tests (links to the corresponding issues) and a new whatsnew message.

@jreback jreback merged commit 91d1c50 into pandas-dev:master Nov 18, 2018
@jreback
Copy link
Contributor

jreback commented Nov 18, 2018

thanks @meiermark keep em coming!

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
@meiermark @tm9k1
BUG: Index.str.partition not nan-safe (pandas-dev#23558) (pandas-dev#…
d0151db 
…23618)
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@meiermark @Pingviinituutti
BUG: Index.str.partition not nan-safe (pandas-dev#23558) (pandas-dev#…
24cd841 
…23618)
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@meiermark @Pingviinituutti
BUG: Index.str.partition not nan-safe (pandas-dev#23558) (pandas-dev#…
c72f05a 
…23618)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfyoung gfyoung gfyoung left review comments

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Bug Indexing Related to indexing on series/frames, not to indexes themselves Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Strings String extension data type and string data
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.

API/BUG: Index.str.split(expand=True) not nan-safe BUG: Index.str.partition not nan-safe
5 participants
@meiermark @pep8speaks @jreback @h-vetinari @gfyoung