Skip to content

REGR: exceptions not caught in _call_map_locations #34113

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

Merged
merged 3 commits into from
May 15, 2020
Merged

REGR: exceptions not caught in _call_map_locations #34113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cfa6be4
REGR: exceptions not caught in _call_map_locations
simonjayhawkins May 11, 2020
3f57b32
Merge remote-tracking branch 'upstream/1.0.x' into describe-regression
simonjayhawkins May 14, 2020
d094d92
Merge remote-tracking branch 'upstream/1.0.x' into describe-regression
simonjayhawkins May 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.0.4.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Fixed regressions
- Bug where an ordered :class:`Categorical` containing only ``NaN`` values would raise rather than returning ``NaN`` when taking the minimum or maximum (:issue:`33450`)
- Bug in :meth:`DataFrameGroupBy.agg` with dictionary input losing ``ExtensionArray`` dtypes (:issue:`32194`)
- Fix to preserve the ability to index with the "nearest" method with xarray's CFTimeIndex, an :class:`Index` subclass (`pydata/xarray#3751 <https://github.com/pydata/xarray/issues/3751>`_, :issue:`32905`).
- Fix regression in :meth:`DataFrame.describe` raising ``TypeError: unhashable type: 'dict'`` (:issue:`32409`)
- Bug in :meth:`DataFrame.replace` casts columns to ``object`` dtype if items in ``to_replace`` not in values (:issue:`32988`)
-

Expand Down
4 changes: 2 additions & 2 deletions pandas/_libs/index.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ cdef class IndexEngine:

self.need_unique_check = 0

cdef void _call_map_locations(self, values):
cpdef _call_map_locations(self, values):
self.mapping.map_locations(values)

def clear_mapping(self):
Expand Down Expand Up @@ -509,7 +509,7 @@ cdef class PeriodEngine(Int64Engine):
cdef _get_index_values(self):
return super(PeriodEngine, self).vgetter()

cdef void _call_map_locations(self, values):
cpdef _call_map_locations(self, values):
Copy link
Member

Choose a reason for hiding this comment

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

these are still cdef on master, why is this the appropriate fix on 1.0.4?

Copy link
Member Author

Choose a reason for hiding this comment

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

why is this the appropriate fix on 1.0.4?

#29232 was a clean-up PR which caused a regression. this is reverting those changes. (applied directly to 1.0.x branch and not touching master, see #34048 (comment))

these are still cdef on master

that's my question to you in the OP.

Could there still be issue on master with the exceptions not being caught?

Copy link
Member

Choose a reason for hiding this comment

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

Could there still be issue on master with the exceptions not being caught?

I can't reproduce the bugs from #32409. I'm guessing you bisected to find the commit that introduced (reintroduced?) the bug, did you find what commit fixed it?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep fixed on master, see #32409 (comment)

but not sure whether the PR that fixed it #31294 should be backported. I can't immediately see the relation.

I see this PR as the safer option as it's a simple revert.

Copy link
Member

Choose a reason for hiding this comment

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

I see this PR as the safer option as it's a simple revert.

Works for me

# super(...) pattern doesn't seem to work with `cdef`
Int64Engine._call_map_locations(self, values.view('i8'))

Expand Down
2 changes: 1 addition & 1 deletion pandas/_libs/index_class_helper.pxi.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ cdef class {{name}}Engine(IndexEngine):
raise KeyError(val)
{{endif}}

cdef void _call_map_locations(self, values):
cpdef _call_map_locations(self, values):
# self.mapping is of type {{hashtable_name}}HashTable,
# so convert dtype of values
self.mapping.map_locations(algos.ensure_{{hashtable_dtype}}(values))
Expand Down