Skip to content

REF: Rename mode.nullable_backend to mode.dtype_backend #50291

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

Merged
merged 6 commits into from
Dec 22, 2022

Conversation

mroeschke
Copy link
Member

From the dev call yesterday, it was discussed to make the new global configuration for 2.0 mode.nullable_backend="pyarrow"|"pandas" more generically named. I have changed it to mode.dtype_backend as we may decide to not always associate this option with the use_nullable_dtype keyword argument in the IO readers

cc @phofl @jrbourbeau

@mroeschke mroeschke added Refactor Internal refactoring of code Arrow pyarrow functionality labels Dec 16, 2022
Copy link
Contributor

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @mroeschke!

FWIW mode.dtype_backend makes sense to me and seems like an improvement over mode.nullable_backend. It might also be worth considering mode.dtypes, which would be nice because of it's conciseness, though maybe it's too concise. I'm curious to hear what you and others think.

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

We should make the docs clearer. nullable_backend was more in line with use_nullable_dtypes. But dtype_backend sounds like a global flag, but as of right now the flag is only pulled if use_nullable_dtypes is True as well.

@jrbourbeau
Copy link
Contributor

@phofl any thoughts on dtypes_backend vs. dtypes vs. some other name?

@phofl
Copy link
Member

phofl commented Dec 16, 2022

The nice thing about the old name: It implies that you have to use nullable dtypes that the option can take effect. If you don't use nullable_dtypes then nullable_backend does not make much sense.

The new name implies that it can work globally, which is not possible right now. In general, I'd be ok with this, but we have to clarify in the docs that this works only in specific situations right now

@mroeschke
Copy link
Member Author

Agreed with the concern, at least for 2.0 where we will be paring this closely with nullable types, but I like that this change future-proofs this option to be more applicable to the non-nullable types in the future

@@ -539,7 +539,7 @@ def use_inf_as_na_cb(key) -> None:
The default storage for StringDtype.
"""

nullable_backend_doc = """
dtype_backend_doc = """
: string
The nullable dtype implementation to return.
Copy link
Member

Choose a reason for hiding this comment

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

I am ok with this too, but I'd like to make this more prominent here that this only works with use_nullable_dtypes as of right now

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's also used with convert_dtypes() now and will (hopefully) be used in more places in the future. Instead of explicitly listing where this option is used, maybe we have some more generic statement about it still being experimental and only used in certain methods -- see the API docs for a specific method to see if it supports this option?

Copy link
Member

Choose a reason for hiding this comment

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

Yep this sounds good to me. I am happy as long as we clarify that it is not generic yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Clarified here that it's only applicable where documented

@mroeschke mroeschke added this to the 2.0 milestone Dec 21, 2022
@phofl phofl merged commit fabdd5d into pandas-dev:main Dec 22, 2022
@phofl
Copy link
Member

phofl commented Dec 22, 2022

thx @mroeschke

Copy link
Contributor

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arrow pyarrow functionality Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants