Skip to content

DOC: don't teach infer_datetime_format in user_guide #50334

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 5 commits into from
Dec 21, 2022

Conversation

ShashwatAgrawal20
Copy link
Contributor

@ShashwatAgrawal20 ShashwatAgrawal20 commented Dec 18, 2022

  • edited the file doc/user_guide/io.rst.

Just removed the infer_datetime_format from the docs.

It's my first pull request & it's not fully complete, so can you guide me on what else changes should be made.

closes #35296

@ShashwatAgrawal20 ShashwatAgrawal20 marked this pull request as draft December 18, 2022 05:05
@@ -272,9 +272,6 @@ parse_dates : boolean or list of ints or names or list of lists or dict, default

.. note::
A fast-path exists for iso8601-formatted dates.
infer_datetime_format : boolean, default ``False``
Copy link
Member

Choose a reason for hiding this comment

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

Hi, we can't simply remove it. We want to add a deprecation warning before removing it when we enforce the actual deprecation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I asked for it in the issue but no one replied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phofl should this be Ok 71466b6

Copy link
Member

Choose a reason for hiding this comment

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

Hey - this parameter is now a no-op, so I'd say that it's correct to remove it from the user guide

It still shows in the docstring as deprecated, but I don't think there's any need to teach it in the user guide

Thoughts @phofl ?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer mentioning that it is a no op but to keep it in here with a deprecation warning anyway. This should increase visibility, which is helpful with deprecations ( I think). Also keeps us consistent with what we are doing for other parameters in read_csv

Copy link
Member

Choose a reason for hiding this comment

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

Sure but does a no-op need this much visibility? I think it looks confusing to beginners to read the user guide and find "you can use this parameter to speed things up. Though actually, never mind, it's deprecated, don't use it"

Ok for docstring, it's just for the user guide that I'm questioning the practice

I'll check tomorrow what's been done for other deprecations anyway, and what other libraries do

And sorry @ShashwatAgrawal20 for conflicting instructions here ☺

@ShashwatAgrawal20
Copy link
Contributor Author

@MarcoGorelli any updates?

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

How about:

  • we mark it as deprecated in the list of parameters (as was done for squeeze in the example I linked)
  • we remove it from the section that teaches about what to try performance-wise, as it's no longer relevant (the deprecation is about how the parameter will be removed, the behaviour has already changed)

Comment on lines 276 to 275
.. note::
The ``infer_datetime_format`` is deprecated and will be removed in the future versions.
infer_datetime_format : boolean, default ``False``
Copy link
Member

Choose a reason for hiding this comment

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

check here for how to mark as deprecated

squeeze : boolean, default ``False``
If the parsed data only contains one column then return a ``Series``.
.. deprecated:: 1.4.0
Append ``.squeeze("columns")`` to the call to ``{func_name}`` to squeeze
the data.

Comment on lines 919 to 922
1. Try to infer the format using ``infer_datetime_format=True`` (see section below).
1. Try to infer the format using ``infer_datetime_format=True`` (this is deprecated and will be removed in the future versions.).
Copy link
Member

@MarcoGorelli MarcoGorelli Dec 19, 2022

Choose a reason for hiding this comment

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

I'd remove it from here

Comment on lines 6386 to 6389
24458940 Oct 10 06:44 test_table_compress.hdf
24458940 Oct 10 06:44 test_table_compress.hdf
Copy link
Member

Choose a reason for hiding this comment

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

not sure what's happened with the line ending - can you run the pre-commit checks on the files you've modified?

@phofl
Copy link
Member

phofl commented Dec 19, 2022

Yeah sorry if I wasn't clear enough. That's exactly what I wanted to do

@ShashwatAgrawal20 ShashwatAgrawal20 requested review from MarcoGorelli and removed request for phofl December 20, 2022 14:09
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for updating

could you set a descriptive title please?

Comment on lines 279 to 283
.. deprecated:: 1.5.2
``infer_datetime_format`` be deprecated (as a strict version of it will become the default);
an easy workaround for non-strict parsing be clearly documented.

Currently, the only way to ensure consistent parsing is by explicitly passing
Copy link
Member

Choose a reason for hiding this comment

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

could you copy the deprecation message from https://pandas.pydata.org/docs/dev/reference/api/pandas.to_datetime.html please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarcoGorelli
are you talking about
image

Copy link
Member

Choose a reason for hiding this comment

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

yup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I be directly using it as it is?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@MarcoGorelli MarcoGorelli changed the title docs: user_guide io DOC: don't teach infer_datetime_format in user_guide Dec 20, 2022
@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 20, 2022
@MarcoGorelli MarcoGorelli marked this pull request as ready for review December 21, 2022 12:04
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

@MarcoGorelli MarcoGorelli merged commit 8469f74 into pandas-dev:main Dec 21, 2022
@ShashwatAgrawal20
Copy link
Contributor Author

@MarcoGorelli thanks!
Had a great experience working with you and learned a lot.

@ShashwatAgrawal20 ShashwatAgrawal20 deleted the doc-date-parser branch May 11, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Reccomended use of read_csv's date_parser parameter is very slow
3 participants