Skip to content

Conversation

jbrockmendel
Copy link
Member

Orthogonal to other outstanding maybe_promote PRs.

This one required pretty significant changes to the code. Using np.min_scalar_type and np.can_cast cleans this up a bit, but it is still pretty verbose. AFAICT there is no clear way to make it shorter without significantly sacrificing clarity.

In a follow-up I think L410-459 can be refactored out to a helper function. Waiting on that until I figure out the boxed=True cases, which are still troublesome.

@jbrockmendel jbrockmendel changed the title TST: FIx xfails for non-box maybe_promote on integer dtypes TST: Fix xfails for non-box maybe_promote on integer dtypes Oct 9, 2019
if res_type.__name__ == "uint64":
# No idea why, but these (sometimes) do not compare as equal
assert ex_type.__name__ == "uint64"
elif res_type.__name__ == "ulonglong":
Copy link
Member Author

Choose a reason for hiding this comment

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

@h-vetinari did you find a nicer way to handle these?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I didn't have to deal with ulonglong (pretty sure that's platform-specific).

Copy link
Contributor

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Integer promotion is a giant headache, haha. :)

ex_type = type(expected_fill_value)
assert res_type == ex_type
if res_type.__name__ == "uint64":
# No idea why, but these (sometimes) do not compare as equal
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is numpy/numpy#12525 and related issues

if res_type.__name__ == "uint64":
# No idea why, but these (sometimes) do not compare as equal
assert ex_type.__name__ == "uint64"
elif res_type.__name__ == "ulonglong":
Copy link
Contributor

Choose a reason for hiding this comment

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

No, I didn't have to deal with ulonglong (pretty sure that's platform-specific).

@jreback jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Oct 10, 2019
@jreback jreback added this to the 1.0 milestone Oct 10, 2019
@jreback jreback merged commit 9a3e1ef into pandas-dev:master Oct 10, 2019
@jreback
Copy link
Contributor

jreback commented Oct 10, 2019

thanks. I suspect this is technically an api internally, but we likely don't expose this to the user. This may also have some perf implications. But let's refactor this beast and remove the array stuff before doing any of that.

@jbrockmendel
Copy link
Member Author

Yah definitely time for a refactor/cleanup pass

@jbrockmendel jbrockmendel deleted the maybe_promote16 branch October 10, 2019 13:34
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Dtype Conversions Unexpected or buggy dtype conversions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants