DOC: Clarify documentation of 'ambiguous' parameter #23408

bartaelterman · 2018-10-29T10:29:22Z

when first reading the docs, the ambiguous parameter was not clear to me. I propose to add a line of documentation and include an example that exactly indicates the issue that the ambiguous parameter solves.

…_locaclize

pep8speaks · 2018-10-29T10:29:26Z

Hello @bartaelterman! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/arrays/datetimes.py !
There are no PEP8 issues in the file pandas/core/generic.py !
There are no PEP8 issues in the file pandas/core/indexes/datetimes.py !

Comment last updated on October 31, 2018 at 23:04 Hours UTC

codecov · 2018-10-29T11:05:40Z

Codecov Report

Merging #23408 into master will increase coverage by 0.03%.
The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master   #23408      +/-   ##
==========================================
+ Coverage   92.18%   92.21%   +0.03%     
==========================================
  Files         161      161              
  Lines       51160    51173      +13     
==========================================
+ Hits        47161    47190      +29     
+ Misses       3999     3983      -16

Flag	Coverage Δ
#multiple	`90.65% <96.29%> (+0.03%)`	⬆️
#single	`42.23% <22.22%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`96.43% <ø> (ø)`	⬆️
pandas/core/arrays/datetimes.py	`97.9% <ø> (ø)`	⬆️
pandas/core/generic.py	`96.81% <96.29%> (+0.02%)`	⬆️
pandas/core/series.py	`93.87% <0%> (-0.02%)`	⬇️
pandas/core/dtypes/base.py	`100% <0%> (ø)`	⬆️
pandas/util/_decorators.py	`91.34% <0%> (ø)`	⬆️
pandas/core/indexes/multi.py	`95.46% <0%> (ø)`	⬆️
pandas/core/groupby/ops.py	`96.79% <0%> (ø)`	⬆️
pandas/core/groupby/base.py	`91.83% <0%> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 184bcd3...b1026c3. Read the comment docs.

pandas/core/generic.py

mroeschke · 2018-10-29T17:02:15Z

Could you copy this explanation to the other occurrences of this argument? It needs to be added to Timestamp, DatetimeIndex and NaT

jorisvandenbossche · 2018-10-29T18:58:23Z

@bartaelterman Thanks for the PR!

In addition, what would also be very useful is an actual code example in an Examples section (feel free to add one in this PR, but also don't feel obliged, the PR is already an improvement)

bartaelterman · 2018-10-29T21:12:30Z

Thanks for your feedback! I will wait for your reply on my comment above after which I will copy over the explanation to the other places. I'll look into the Examples section too.

…x and NaT This is following the discussion at pandas-dev#23408 (comment)

bartaelterman · 2018-10-30T13:20:06Z

@mroeschke I've updated the doc as discussed and added it to Timestamp, DatetimeIndex and NaT.
@jorisvandenbossche I've added a couple of examples. However I'm not comfortable with the Cython stuff...
If you have more remarks, I am happy to modify it further.

bartaelterman · 2018-10-30T20:42:55Z

So the docstring test for NaT have failed but I'm not sure what's wrong:

=================================== FAILURES ===================================
_____________________________ test_NaT_docstrings ______________________________

    def test_NaT_docstrings():
        # GH#17327
        nat_names = dir(NaT)
    
        # NaT should have *most* of the Timestamp methods, with matching
        # docstrings.  The attributes that are not expected to be present in NaT
        # are private methods plus `ts_expected` below.
        ts_names = dir(Timestamp)
        ts_missing = [x for x in ts_names if x not in nat_names and
                      not x.startswith('_')]
        ts_missing.sort()
        ts_expected = ['freqstr', 'normalize',
                       'to_julian_date',
                       'to_period', 'tz']
        assert ts_missing == ts_expected
    
        ts_overlap = [x for x in nat_names if x in ts_names and
                      not x.startswith('_') and
                      callable(getattr(Timestamp, x))]
        for name in ts_overlap:
            tsdoc = getattr(Timestamp, name).__doc__
            natdoc = getattr(NaT, name).__doc__
>           assert tsdoc == natdoc
E           AssertionError: assert '\n        Co...ne.\n        ' == '\n        Con...ne.\n        '
E             Skipping 383 identical leading characters in diff, use -v to show
E             -           When clocks moved backward due to DST, ambiguous times may arise.
E             -             For example in Central European Time (UTC+01), when going from 03:00 DST
E             -             to 02:00 non-DST, 02:30:00 local time occurs both at 00:30:00 UTC
E             -             and at 01:30:00 UTC. In such a situation, the `ambiguous` parameter
E             -             dictates how ambiguous times should be handled.
E             - 
E             -             - bool contains flags to determine if time is dst or not (note
E             ? --
E             +           - bool contains flags to determine if time is dst or not (note
E                             that this flag is only applicable for ambiguous fall dst dates)
E                           - 'NaT' will return NaT for an ambiguous time
E                           - 'raise' will raise an AmbiguousTimeError for an ambiguous time
E               
E                       nonexistent : 'shift', 'NaT', default 'raise'
E                           A nonexistent time does not exist in a particular timezone
E                           where clocks moved forward due to DST.
E               
E                           - 'shift' will shift the nonexistent time forward to the closest
E                             existing time
E                           - 'NaT' will return NaT where there are nonexistent times
E                           - 'raise' will raise an NonExistentTimeError if there are
E                             nonexistent times
E               
E                           .. versionadded:: 0.24.0
E               
E                       errors : 'raise', 'coerce', default None
E                           - 'raise' will raise a NonExistentTimeError if a timestamp is not
E                              valid in the specified timezone (e.g. due to a transition from
E                              or to DST time). Use ``nonexistent='raise'`` instead.
E                           - 'coerce' will return NaT if the timestamp can not be converted
E                             into the specified timezone. Use ``nonexistent='NaT'`` instead.
E               
E                             .. deprecated:: 0.24.0
E               
E                       Returns
E                       -------
E                       localized : Timestamp
E               
E                       Raises
E                       ------
E                       TypeError
E                           If the Timestamp is tz-aware and tz is not None.

pandas/tests/scalar/test_nat.py:185: AssertionError

mroeschke · 2018-10-30T21:27:51Z

@bartaelterman in essence, the docstrings for NaT need to be the same as the docstrings for Timestamp, so you need to add your explanation of ambiguous in nattype.pyx for the tz_localize defined there.

pandas/core/generic.py

pandas/core/indexes/datetimes.py

bartaelterman · 2018-10-31T06:53:00Z

Should I add the docstring also to core/arrays/datetimes.py? And in that case, shall I add the example with dt.tz_localize() there? It seems to me that would end up here which seems appropriate.

mroeschke · 2018-10-31T15:28:16Z

Yes that'd be great @bartaelterman.

The examples look good. Just looks like there's a doctest failure and pep8 issues.

bartaelterman · 2018-10-31T20:09:40Z

The Travis build is still failing, but I'm not sure why. All tests seem to pass and I don't see an error message, except at the very end:
The command "ci/code_checks.sh" exited with 2.
Any ideas what's going wrong? Thanks

TomAugspurger · 2018-10-31T20:17:33Z

see https://travis-ci.org/pandas-dev/pandas/jobs/448947238#L2763

Some lines are too long.

TomAugspurger · 2018-11-02T17:03:43Z

pandas/_libs/tslibs/conversion.pyx

+          non-DST time (note that this flag is only applicable for ambiguous
+          times, but the array must have the same length as vals)
+        - bool if True, treat all vals as DST. If False, treat them as non-DST
+        - 'NaT' will return NaT where there are ambiguous times


Is this the string 'NaT', or the singleton pd.NaT (or either?) Can we clarify that?

It seems like the allowed types are a bit inconsistent among all these functions. Can you check whether that's the docs being wrong? or are the just different?

This will return the pd.NaT singleton.

I was wondering about whether the input should be pd.NaT or 'NaT'.

Oh sorry, ambiguous accepts the input 'NaT' and should raise on pd.NaT

datapythonista

lgtm

But we should check whether the ambiguos accepts different values ni each function, or it's the documentation that doesn't document all cases. But it can be in a separate PR I think.

TomAugspurger · 2018-11-04T13:56:13Z

Agreed. This is a useful change as is.

* Add documentation line with example for the ambiguous parameter of tz_locaclize * Updating 'ambiguous'-param doc + update it on Timestamp, DatetimeIndex and NaT This is following the discussion at pandas-dev#23408 (comment)

Add documentation line with example for the ambiguous parameter of tz…

f25ed6b

…_locaclize

gfyoung added Docs Datetime Datetime data dtype labels Oct 29, 2018

gfyoung assigned jreback and mroeschke and unassigned jreback and mroeschke Oct 29, 2018

gfyoung requested review from mroeschke and jreback October 29, 2018 13:37

mroeschke reviewed Oct 29, 2018

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

mroeschke reviewed Oct 29, 2018

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

mroeschke reviewed Oct 29, 2018

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

bartaelterman added 2 commits October 30, 2018 09:30

Updating 'ambiguous'-param doc + update it on Timestamp, DatetimeInde…

551e149

…x and NaT This is following the discussion at pandas-dev#23408 (comment)

Add some examples for the ambiguous parameter for localizing timestamps

4274d08

mroeschke reviewed Oct 30, 2018

View reviewed changes

pandas/core/generic.py Show resolved Hide resolved

mroeschke reviewed Oct 30, 2018

View reviewed changes

pandas/core/indexes/datetimes.py Outdated Show resolved Hide resolved

mroeschke reviewed Oct 30, 2018

View reviewed changes

pandas/core/indexes/datetimes.py Outdated Show resolved Hide resolved

mroeschke reviewed Oct 30, 2018

View reviewed changes

pandas/core/indexes/datetimes.py Outdated Show resolved Hide resolved

bartaelterman added 2 commits October 31, 2018 07:24

Make docstring of NaT tz_localize equal to Timestamp tz_localize

b33e370

show example for Series.tz_localize (not dt.tz_localize)

a7b27b6

bartaelterman added 2 commits October 31, 2018 17:17

add ambiguous docstring to arrays/datetimes

efeeab7

fix bug in the docstring

7e60b3d

bartaelterman added 3 commits October 31, 2018 21:24

chop off long lines in docstring

d18e80f

chop long lines in docstring

4a9ad4b

chop long line in docstring

b1026c3

TomAugspurger reviewed Nov 2, 2018

View reviewed changes

datapythonista approved these changes Nov 4, 2018

View reviewed changes

datapythonista changed the title ~~Add documentation line with example for the ambiguous parameter~~ DOC: Clarify documentation of 'ambiguous' parameter Nov 4, 2018

TomAugspurger merged commit 07426af into pandas-dev:master Nov 4, 2018

bartaelterman deleted the add_tz_localize_doc branch November 4, 2018 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Clarify documentation of 'ambiguous' parameter #23408

DOC: Clarify documentation of 'ambiguous' parameter #23408

bartaelterman commented Oct 29, 2018

pep8speaks commented Oct 29, 2018 •

edited

Loading

codecov bot commented Oct 29, 2018 •

edited

Loading

mroeschke commented Oct 29, 2018

jorisvandenbossche commented Oct 29, 2018

bartaelterman commented Oct 29, 2018

bartaelterman commented Oct 30, 2018

bartaelterman commented Oct 30, 2018

mroeschke commented Oct 30, 2018

bartaelterman commented Oct 31, 2018

mroeschke commented Oct 31, 2018

bartaelterman commented Oct 31, 2018

TomAugspurger commented Oct 31, 2018

TomAugspurger Nov 2, 2018

TomAugspurger Nov 2, 2018

mroeschke Nov 2, 2018

TomAugspurger Nov 2, 2018

mroeschke Nov 2, 2018 •

edited

Loading

datapythonista left a comment

TomAugspurger commented Nov 4, 2018

DOC: Clarify documentation of 'ambiguous' parameter #23408

DOC: Clarify documentation of 'ambiguous' parameter #23408

Conversation

bartaelterman commented Oct 29, 2018

pep8speaks commented Oct 29, 2018 • edited Loading

Comment last updated on October 31, 2018 at 23:04 Hours UTC

codecov bot commented Oct 29, 2018 • edited Loading

Codecov Report

mroeschke commented Oct 29, 2018

jorisvandenbossche commented Oct 29, 2018

bartaelterman commented Oct 29, 2018

bartaelterman commented Oct 30, 2018

bartaelterman commented Oct 30, 2018

mroeschke commented Oct 30, 2018

bartaelterman commented Oct 31, 2018

mroeschke commented Oct 31, 2018

bartaelterman commented Oct 31, 2018

TomAugspurger commented Oct 31, 2018

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

mroeschke Nov 2, 2018

Choose a reason for hiding this comment

TomAugspurger Nov 2, 2018

Choose a reason for hiding this comment

mroeschke Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

TomAugspurger commented Nov 4, 2018

pep8speaks commented Oct 29, 2018 •

edited

Loading

codecov bot commented Oct 29, 2018 •

edited

Loading

mroeschke Nov 2, 2018 •

edited

Loading