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

Make kwargs explicit in put, append #29957

Merged
merged 9 commits into from
Dec 4, 2019

Conversation

jbrockmendel
Copy link
Member

Made possible by #29951, progress towards #20903

@jreback input needed on whether certain not-covered-in-tests cases should be considered possible. See that both put and append still contain **kwargs. In all extant tests, kwargs is always empty. But there are remaining keywords that write_to_group (which both put and append call) accepts that are not in the put/append signatures.

So the question is: is there a reason why put/append would accept different subsets of the writing keywords? If not, they should both have the same explicit kwargs as write_to_group.

Once this is pinned down, we can get explicit all the way up the stack to NDFrame.to_hdf

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

@jbrockmendel lgtm couple of suggestions, not blockers.

except KeyError:
raise TypeError(f"invalid HDFStore format specified [{format}]")

return kwargs
return format
Copy link
Member

Choose a reason for hiding this comment

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

could just return in try block and remove intermediate variable?

@@ -2412,6 +2412,8 @@ def to_hdf(
complib: Optional[str] = None,
append: bool_t = False,
format: Optional[str] = None,
min_itemsize=None,
Copy link
Member

Choose a reason for hiding this comment

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

could these keywords added here and in #29939 for the public API be keyword only, xref #27544

Copy link
Member Author

Choose a reason for hiding this comment

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

i like this idea, will do in a dedicated PR

@simonjayhawkins simonjayhawkins added the IO HDF5 read_hdf, HDFStore label Dec 2, 2019
@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Dec 2, 2019
@jreback
Copy link
Contributor

jreback commented Dec 2, 2019

Made possible by #29951, progress towards #20903

@jreback input needed on whether certain not-covered-in-tests cases should be considered possible. See that both put and append still contain **kwargs. In all extant tests, kwargs is always empty. But there are remaining keywords that write_to_group (which both put and append call) accepts that are not in the put/append signatures.

So the question is: is there a reason why put/append would accept different subsets of the writing keywords? If not, they should both have the same explicit kwargs as write_to_group.

Once this is pinned down, we can get explicit all the way up the stack to NDFrame.to_hdf

these are likely ok. The reason for extra keywords is mainly on opening of the store itself, where PyTables passes thru keywords (e.g. to specify a different kind of backend: https://github.com/PyTables/PyTables/blob/master/tables/file.py#L219)

@jbrockmendel
Copy link
Member Author

these are likely ok. The reason for extra keywords is mainly on opening of the store itself,

To be clear, I'm referring to keywords that we have established are accepted by _write_to_group, i.e. not ones being passed to HDFStore.

@@ -2412,6 +2412,8 @@ def to_hdf(
complib: Optional[str] = None,
append: bool_t = False,
format: Optional[str] = None,
min_itemsize=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is Union[Optional[int], Dict[str, int]]

@@ -2412,6 +2412,8 @@ def to_hdf(
complib: Optional[str] = None,
append: bool_t = False,
format: Optional[str] = None,
min_itemsize=None,
data_columns=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional[List[str]]

@@ -2471,6 +2473,8 @@ def to_hdf(
See the errors argument for :func:`open` for a full list
of options.
encoding : str, default "UTF-8"
min_itemsize : dict, optional
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

data_columns=data_columns,
errors=errors,
encoding=encoding,
**kwargs,
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah ok with removing the kwargs

@jreback
Copy link
Contributor

jreback commented Dec 2, 2019

To be clear, I'm referring to keywords that we have established are accepted by _write_to_group, i.e. not ones being passed to HDFStore.

then i think ok to remove

@jbrockmendel
Copy link
Member Author

Updated:

  • updated annotations
  • removed kwargs
  • to do this, had to enforce nan_rep cannot be passed with append=True (double check this is desired)?
  • removed no-longer-allowed dropna, fletcher32 kwargs from NDFrame.to_hdf docstring (double check this is desired)?

@jbrockmendel
Copy link
Member Author

updated+green (ex an unrelated jinja2 problem ill work on in the morning)

@jreback jreback merged commit 8f0310a into pandas-dev:master Dec 4, 2019
@jreback
Copy link
Contributor

jreback commented Dec 4, 2019

thanks

@jbrockmendel jbrockmendel deleted the cln-pytables-kwargs4 branch December 4, 2019 16:40
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@simonjayhawkins simonjayhawkins mentioned this pull request Aug 15, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO HDF5 read_hdf, HDFStore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants