Skip to content
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: Fix series rename called with str altering name rather index (GH17407) #17654

Merged
merged 8 commits into from
Sep 30, 2017

Conversation

huashuai
Copy link
Contributor

@codecov
Copy link

codecov bot commented Sep 24, 2017

Codecov Report

Merging #17654 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17654      +/-   ##
==========================================
- Coverage   91.27%   91.22%   -0.05%     
==========================================
  Files         163      163              
  Lines       49765    49766       +1     
==========================================
- Hits        45421    45401      -20     
- Misses       4344     4365      +21
Flag Coverage Δ
#multiple 89.02% <100%> (-0.03%) ⬇️
#single 40.33% <100%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/core/dtypes/inference.py 98.36% <100%> (+0.02%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.38% <0%> (-1.82%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️

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 e2a0251...a1f9565. Read the comment docs.

@@ -263,7 +263,8 @@ def is_list_like(obj):
"""

return (hasattr(obj, '__iter__') and
not isinstance(obj, string_and_binary_types))
Copy link
Contributor

@jreback jreback Sep 24, 2017

Choose a reason for hiding this comment

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

do this with an or between iter and collections....

return (hasattr(obj, '__iter__') or isinstance(obj, Iterable) and not isinstance(obj, string....)

Copy link
Contributor Author

@huashuai huashuai Sep 24, 2017

Choose a reason for hiding this comment

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

using or doesn't work, as hasattr(str, '__iter__') returns True and that caused the bug. I changed it to use and

@@ -263,7 +263,8 @@ def is_list_like(obj):
"""

return (hasattr(obj, '__iter__') and
not isinstance(obj, string_and_binary_types))
not isinstance(obj, string_and_binary_types) and
isinstance(obj, collections.Iterable))
Copy link
Contributor

@jreback jreback Sep 24, 2017

Choose a reason for hiding this comment

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

from collection import Iterable directly to the name space at the top

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jreback jreback added the Bug label Sep 24, 2017
@@ -546,6 +546,7 @@ Indexing
- Bug in intersection of ``RangeIndex`` with negative step (:issue:`17296`)
- Bug in ``IntervalIndex`` where performing a scalar lookup fails for included right endpoints of non-overlapping monotonic decreasing indexes (:issue:`16417`, :issue:`17271`)
- Bug in :meth:`DataFrame.first_valid_index` and :meth:`DataFrame.last_valid_index` when no valid entry (:issue:`17400`)
- Bug in ``Series.rename`` when called with `str` alters name of series rather than index of series. (:issue:`17407`)
Copy link
Contributor

Choose a reason for hiding this comment

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

called with a callable

@@ -263,7 +263,8 @@ def is_list_like(obj):
"""

return (hasattr(obj, '__iter__') and
not isinstance(obj, string_and_binary_types))
not isinstance(obj, string_and_binary_types) and
isinstance(obj, collections.Iterable))


Copy link
Contributor

Choose a reason for hiding this comment

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

pls run a sub-set of the perf tests (IOW something like this)

asv continuous master thisbranchname --bench indexing
asv continuous master thisbranchname --bench timeseries

and report any significant diffs.

This touches code from almost everywhere indirectly so want to make sure no regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. See comments below for what the performance tests reported.

@huashuai
Copy link
Contributor Author

huashuai commented Sep 24, 2017

For indexing performance test,

   before           after         ratio
 [f797c1dc]       [ccb3887a]
 4.05±0.01μs      3.58±0.02μs     0.88  indexing.DataFrameIndexing.time_get_value

For timeseries performance test,

   before           after         ratio
 [f797c1dc]       [ccb3887a]
    3.68s            3.27s     0.89  timeseries.Iteration.time_iter_periodindex

@@ -262,7 +263,7 @@ def is_list_like(obj):
False
"""

return (hasattr(obj, '__iter__') and
return (hasattr(obj, '__iter__') and isinstance(obj, Iterable) and
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be an or

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but that wont fix the issue. What about only check isinstance(obj, Iterable)? I'm asking this because I see another function in inference.py called _iterable_not_string, it checks against collections.Iterable rather check for __iter__ attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can try but this is used pretty much everywhere and you will break things. Technically a type satisfies this condition I think, but we DO want to exlude it. you could also add it to the not isinstance(obj, (string_types, type)). might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked, type actually doesn't satisfy the condition. Also all the tests have passed after the change.

@@ -565,6 +565,7 @@ Indexing
- Bug in intersection of ``RangeIndex`` with negative step (:issue:`17296`)
- Bug in ``IntervalIndex`` where performing a scalar lookup fails for included right endpoints of non-overlapping monotonic decreasing indexes (:issue:`16417`, :issue:`17271`)
- Bug in :meth:`DataFrame.first_valid_index` and :meth:`DataFrame.last_valid_index` when no valid entry (:issue:`17400`)
- Bug in ``Series.rename`` when called with a `callable` alters name of series rather than index of series. (:issue:`17407`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug in :func:`Series.rename` when called with a callable, incorrectly alters the name of the Series, rather than the name of the Index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jreback jreback added this to the 0.21.0 milestone Sep 25, 2017
@jreback
Copy link
Contributor

jreback commented Sep 28, 2017

can you do

git commit --all --amend -C HEAD

then force push

this will rebuild and make sure our CI passes. ping on green.

@huashuai huashuai force-pushed the fix-17407 branch 2 times, most recently from a61c12b to 5d8bbf9 Compare September 29, 2017 21:35
@huashuai
Copy link
Contributor Author

ci failed with this error:

unable to execute 'x86_64-conda_cos6-linux-gnu-gcc': No such file or directory

and I found this on stackoverflow, seems related to a new Anaconda release.

Any idea how to fix this?

@TomAugspurger
Copy link
Contributor

If you rebase on top of master it should be fixed.

@huashuai
Copy link
Contributor Author

Thanks, Tom! CI passes now.

@TomAugspurger TomAugspurger merged commit 030e374 into pandas-dev:master Sep 30, 2017
@TomAugspurger
Copy link
Contributor

Just removed a conflict marker in the whatsnew in a1f9565. Thanks @huashuai

@huashuai
Copy link
Contributor Author

huashuai commented Oct 1, 2017

Thanks @TomAugspurger and @jreback for the reviews. This is my first PR, and I'm excited to do more.

ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 2, 2017
* 'master' of github.com:pandas-dev/pandas: (188 commits)
  Separate out _convert_datetime_to_tsobject (pandas-dev#17715)
  DOC: remove whatsnew note for xref pandas-dev#17131
  BUG: Regression in .loc accepting a boolean Index as an indexer (pandas-dev#17738)
  DEPR: Deprecate cdate_range and merge into bdate_range (pandas-dev#17691)
  CLN: replace %s syntax with .format in pandas.core: categorical, common, config, config_init (pandas-dev#17735)
  Fixed the memory usage explanation of categorical in gotchas from O(nm) to O(n+m) (pandas-dev#17736)
  TST: add backward compat for offset testing for pickles (pandas-dev#17733)
  remove unused time conversion funcs (pandas-dev#17711)
  DEPR: Deprecate convert parameter in take (pandas-dev#17352)
  BUG:Time Grouper bug fix when applied for list groupers (pandas-dev#17587)
  BUG: Fix some PeriodIndex resampling issues (pandas-dev#16153)
  BUG: Fix unexpected sort in groupby (pandas-dev#17621)
  DOC: Fixed typo in documentation for 'pandas.DataFrame.replace' (pandas-dev#17731)
  BUG: Fix series rename called with str altering name rather index (GH17407) (pandas-dev#17654)
  DOC: Add examples for MultiIndex.get_locs + cleanups (pandas-dev#17675)
  Doc improvements for IntervalIndex and Interval (pandas-dev#17714)
  BUG: DataFrame sort_values and multiple "by" columns fails to order NaT correctly
  Last of the timezones funcs (pandas-dev#17669)
  Add missing file to _pyxfiles, delete commented-out (pandas-dev#17712)
  update imports of DateParseError, remove unused imports from tslib (pandas-dev#17713)
  ...
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
…17407) (pandas-dev#17654)

* BUG: Fix series rename called with str altering the name. GH17407

* add whatsnew for the fix for pandas-dev#17407

* Fix typo in whatsnew

* remove whitespace

* Update code after @jreback's comments

* Change `or` to `and` for checking iterable

* Only check against Iterable in is_list_like and add test for `str`

* Update v0.21.0.txt
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
…17407) (pandas-dev#17654)

* BUG: Fix series rename called with str altering the name. GH17407

* add whatsnew for the fix for pandas-dev#17407

* Fix typo in whatsnew

* remove whitespace

* Update code after @jreback's comments

* Change `or` to `and` for checking iterable

* Only check against Iterable in is_list_like and add test for `str`

* Update v0.21.0.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Series#rename(str) change the name of Series.
3 participants