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

API: Clean up dtypes/common.py #16242

Closed
gfyoung opened this issue May 4, 2017 · 9 comments
Closed

API: Clean up dtypes/common.py #16242

gfyoung opened this issue May 4, 2017 · 9 comments
Labels
Clean Closing Candidate May be closeable, needs more eyeballs Dtype Conversions Unexpected or buggy dtype conversions

Comments

@gfyoung
Copy link
Member

gfyoung commented May 4, 2017

When I was developing #16237, I noticed that the API for checking dtype is quite messy. We appear to have duplicate functionality, and the interface is not clear as to what we should accept.

I propose that for all such dtype checking functions, we should accept ANY array-like and ANY dtype OR type object and cast any list-like object to np.array.

Also, I added several TODO in that same file that I was hoping to get some feedback on. They stood out to me because they seemed either non-intuitive OR symptomatic of the API redundancy that I was referring to.

  • is_datetimetz seems like a duplicate of is_datetime64tz_dtype. Do we need it?
  • is_period seems like a duplicate of is_period_arraylike. Do we need it?
  • Should Period be an instance of PeriodDtype?
  • Should Interval be an instance of IntervalDtype?
@jreback
Copy link
Contributor

jreback commented May 4, 2017

I propose that for all such dtype checking functions, we should accept ANY array-like and ANY dtype OR type object and cast any list-like object to np.array.

this is likely not desired. These are for unknown array-like only (e.g. Index,ndarray,Series), which can simply be introspected with NO computation. IOW anything that needs element-by-element detection is a no-no here.

@jreback jreback added the Dtype Conversions Unexpected or buggy dtype conversions label May 4, 2017
@jreback
Copy link
Contributor

jreback commented May 4, 2017

can you post the TODO's here.

@gfyoung
Copy link
Member Author

gfyoung commented May 5, 2017

this is likely not desired. These are for unknown array-like only (e.g. Index,ndarray,Series), which can simply be introspected with NO computation.

I'm confused, something like:

def is_*_dtype(arr_or_dtype):
    if is_list_like(arr_or_dtype) and not hasattr(arr_or_dtype, dtype):
       arr_or_dtype = np.asarray(arr_or_dtype)
...

@gfyoung
Copy link
Member Author

gfyoung commented May 5, 2017

can you post the TODO's here.

I edited the original issue to add them.

@jreback
Copy link
Contributor

jreback commented May 5, 2017

Should Period be an instance of PeriodDtype?
Should Interval be an instance of IntervalDtype?

yes but these would require quite some work
they only exists as Index currently

@gfyoung
Copy link
Member Author

gfyoung commented May 5, 2017

they only exists as Index currently

What do you mean?

@jreback
Copy link
Contributor

jreback commented May 5, 2017

welll see what happens when you store a PeriodIndex as a column

@jbrockmendel
Copy link
Member

@gfyoung closeable? most of the topics mentioned in the OP look like they've been handled

@jbrockmendel jbrockmendel added the Closing Candidate May be closeable, needs more eyeballs label Dec 21, 2021
@mroeschke
Copy link
Member

Yup I think all the initial issues have been address so closing. We can open follow up issues targeting specific function if still necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Closing Candidate May be closeable, needs more eyeballs Dtype Conversions Unexpected or buggy dtype conversions
Projects
None yet
Development

No branches or pull requests

4 participants