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

CLN: What is the purpose of datetools.py? #14094

Closed
gfyoung opened this issue Aug 27, 2016 · 21 comments

Comments

Projects
None yet
3 participants
@gfyoung
Copy link
Member

commented Aug 27, 2016

The doc-string at the top of datetools.py seems to indicate this is just a hodge-podge of datetime-related objects. Seems like that could be cleaned up unless there is good reason to have it.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

xref #5886

yeah I think we had a discussion about this in the past. I think it is appropriate to deprecate this entire module, and point to pd.offsets for imports of these (using the common), and not the 'older' yearBegin-like, which IIRC is something from scikit-timeseries).

I also suspect this is commonly imported by people, so better to put up the deprecation warning now. some of these might be reasonably for the public api, but that's another discussion.

@jorisvandenbossche

@jreback jreback added the Deprecate label Aug 27, 2016

@jreback jreback added this to the 0.19.0 milestone Aug 27, 2016

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

this is ATM an import to pandas itself, so need to remove that (and also from tests where it is). e.g. importing pandas should not generate this warning, only importing it directly should

e.g. from pandas import datetools (you have to put a shim around it)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

pandas/core/api.py:
   32 : import pandas.core.datetools as datetools
@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

@jreback : I'm not entirely familiar with shimming. Explain?

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

Also, I might that it seems this module does expose some objects that could be useful in the public API, but I agree they should be re-organised afterwards.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

like this: https://github.com/pydata/pandas/blob/master/pandas/core/common.py#L23

the idea is you can't directly affect pandas.datetools, instead when someone does

from pandas.datetools import yearBegin they should get the warning. So you wrap all of the exposed functions in datetools.py with the deprecation wrapper that upon usage will show it.

There might be a better way of doing this, to have anything from from pandas.datetools import .... work with the warning (you might need to google how to do this).

IIRC @njsmith has some magic code to do this.

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

@jreback : But there are no functions to wrap here? They're a bunch of convenience objects.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

sure there are

in core/datetools.py

yearEnd = YearEnd()

can easily become

import sys
m = sys.modules['pandas.core.datetools']

def deprecate(name, alt_name):

    def wrapper(*args, **kwargs):
        warnings.warn("the pandas.core.datetools module is deprecated. Use from pandas.offsets import %s instead" % (name, alt_name),
                      FutureWarning, stacklevel=2)
        return m['alt_name'](*args, **kwargs)
    return wrapper

yearBegin = deprecate('yearBegin', 'YearBegin')

may need several mini versions of these to have the correct warning message (e.g. where to import from)

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

Ah, that's a good point.

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

Yep, I think it would be nice to deprecated it. But, we should decide on a proper namespacing for those things (as they are useful public methods/objects).

But it seems it will be difficult to deprecate some of those. Because the objects that start with a lower character are actual instances and not functions/constructors (eg monthEnd = MonthEnd(). So that is usable right now as pd.datetools.monthEnd, which is just an object you do not need to 'call', so I don't think the shim approach works for that?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

it's very odd that they r here to be honest
I don't know the rationale

we need an import hook to depreciate these I think

or a proxy object that shows the deprecation when u use them (like I did with resample)

or maybe just remove them
they are not standard naming schemes are even proper usage

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

Yep, I think it would be nice to deprecated it. But, we should decide on a proper namespacing for those things (as they are useful public methods/objects).

these don't serve any purpose as they are just aggregates imports - I don't thing they should be put anywhere

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

@jreback , @jorisvandenbossche : Why not just deprecate the module like we are doing with rpy2? I think that might be simplest IMO.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

these are already imported into pd.datetools namespace

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

Not sure if we need to go through all those of loops just for this. I suspect a thorough refactoring could do the trick IMO.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

suspect a thorough refactoring could do the trick IMO.

not sure what that means - sure we can just drop the code but better to provide a deprecation

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

Thorough refactoring, move all of these "convenience objects" to other locations, remove all internal references (e.g. tests), etc.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

of course and how do you propose to deprecate?

@gfyoung

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2016

Do what we did with rpy2?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

and how is that going to work?
this is ALREADY imported you can just directly access the namespace

the point is we need to intercept that access and provide a warning - shimming via wrapped functions or a meta module is necessary

gfyoung added a commit to forking-repos/pandas that referenced this issue Sep 3, 2016

jorisvandenbossche added a commit that referenced this issue Sep 7, 2016

DEPR: Deprecate pandas.core.datetools (#14105)
* MAINT: Replace datetools import in tests

* MAINT: Replace datetools import internally

* DOC: Replace datetools import in docs

* MAINT: Remove datetool imports from scripts

* DEPR: Deprecate pandas.core.datetools

Closes gh-14094.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.