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

Tick DateOffset Normalize whatsnew entry has an error #21564

Closed
1 of 2 tasks
TomAugspurger opened this issue Jun 20, 2018 · 14 comments
Closed
1 of 2 tasks

Tick DateOffset Normalize whatsnew entry has an error #21564

TomAugspurger opened this issue Jun 20, 2018 · 14 comments
Labels
Closing Candidate May be closeable, needs more eyeballs Docs Frequency DateOffsets good first issue

Comments

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jun 20, 2018

http://pandas-docs.github.io/pandas-docs-travis/whatsnew.html#tick-dateoffset-normalize-restrictions

@jbrockmendel
Copy link
Member

The second bulletpoint is exactly the point of #21427. Me messing up the whatsnew is entirely plausible. Happy to fix, but will need some guidance, as I clearly don't understand [something].

@uds5501
Copy link
Contributor

uds5501 commented Jun 23, 2018

@TomAugspurger considering that normalize = True was supposed to throw an exception according to #21427, isn't the exception desirable?

@TomAugspurger
Copy link
Contributor Author

If that was the intent, then the .. ipython:: python block needs an :okexcept: so that a warning doesn't bubble up to the doc builder.

We also don't need the tic on its own line, since that just produces

In [4]: tic
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-4-cd137cbd8d43> in <module>()
----> 1 tic

NameError: name 'tic' is not defined

@jorisvandenbossche
Copy link
Member

Repeating my comments from the other issue:

In any case, all occurences of this in the docs should be fixed.

But, if this change is needed, it would also be nice if there is a clear alternative provided in the documentation / deprecation message.
And I think we should maybe also consider deprecating this instead of simply changing.

@jorisvandenbossche
Copy link
Member

So this is the use case in the docs (from http://pandas.pydata.org/pandas-docs/stable/timeseries.html#dateoffset-objects):

In [124]: day = Day()

In [125]: day.apply(pd.Timestamp('2014-01-01 09:00'))
Out[125]: Timestamp('2014-01-02 09:00:00')

In [126]: day = Day(normalize=True)

In [127]: day.apply(pd.Timestamp('2014-01-01 09:00'))
Out[127]: Timestamp('2014-01-02 00:00:00')

What is the alternative?

@jbrockmendel
Copy link
Member

@jorisvandenbossche good catch, I missed that point in the docs.

The alternative is:

In [123] ts = pd.Timestamp('2014-01-01 09:00')

In [124]: day = Day()

In [127]: day.apply(ts.normalize())
Out [127]: Timestamp('2014-01-02 00:00:00')

@jorisvandenbossche
Copy link
Member

OK, that's a good alternative (only for the Hour example, the normalize needs to go after the apply, I think?).
Can you do a PR to update the docs and deprecation message?

@jbrockmendel
Copy link
Member

Can you do a PR to update the docs and deprecation message?

I'll try to get this started over the weekend.

@jorisvandenbossche jorisvandenbossche added the Blocker Blocking issue or pull request for an upcoming release label Jul 8, 2018
@jbrockmendel
Copy link
Member

Looking at timeseries.rst the only example with normalize=True is Day. If we edit that to disallow normalize, we'll need to add another non-Tick example where normalize is allowed. How verbose do we want this section to be?

It may also be worth resolving #20633 before spending too much time re-writing these docs.

@jreback
Copy link
Contributor

jreback commented Nov 6, 2018

is this still open?

@TomAugspurger
Copy link
Contributor Author

Yes, I think so.

@jreback jreback modified the milestones: 0.24.0, Contributions Welcome Dec 2, 2018
@jbrockmendel
Copy link
Member

@TomAugspurger still open? if it was really a blocker for 0.24.0, then I'd imagine it must have been fixed

@jorisvandenbossche
Copy link
Member

The errors in the docs itself have been fixed in 1986dbe and a03d953

My previous comment about that it would have been nicer to deprecate instead of break this, that ship has sailed ;).
It would still be nice to include the alternative in the whatsnew docs though.

@TomAugspurger TomAugspurger removed the Blocker Blocking issue or pull request for an upcoming release label Jun 20, 2019
@mroeschke mroeschke added the Frequency DateOffsets label Jun 20, 2021
@mroeschke mroeschke removed this from the Contributions Welcome milestone Oct 13, 2022
@jbrockmendel
Copy link
Member

Last comment suggests the doc issue has been fixed and the remaining question was about whether to do something as a deprecation or not. Since that was from 2019, i think that ship has sailed. Is this closable?

@jbrockmendel jbrockmendel added the Closing Candidate May be closeable, needs more eyeballs label Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closing Candidate May be closeable, needs more eyeballs Docs Frequency DateOffsets good first issue
Projects
None yet
Development

No branches or pull requests

6 participants