-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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: reorg type inference & introspection #13147
Conversation
There is a slightly problematic aspect here -- other libraries that needed to be able to use |
no that type of dtype comparison isn't safe i can simply import some things back into the pdcom space |
One safe option (the best we came up, if I recall correctly) is hasattr(s, On Wed, May 11, 2016 at 7:15 PM, Jeff Reback notifications@github.com
|
cc @llllllllll |
It might be worth defining a public API for this purpose. Of course the ship has sailed on earlier versions of pandas |
These are also added back to
|
166181e
to
329f44c
Compare
@jreback the only things we have in Zipline that are touching pandas.core are in a utility module that's already doing backwards-compat stuff anyway:
|
@jreback See https://github.com/njsmith/metamodule. It requires some messy tricks to support Python <3.5 though. |
hmm, that might do the trick of deprecating these de-facto apis (though prob will do that in the 0.19.0). |
The messy tricks aren't too bad because they only have to support a specific list of old, non-moving Python versions, and I already went through and checked that all of those versions are handled correctly. All future versions of Python have a clean upstream supported way to do this. But, if you want to avoid that, you actually can -- the really clever stuff in meta module is required to support the case where you need to fix up a module from inside that module (e.g. if you wanted to deprecate to attribute of pandas itself), without rewriting all your API export logic. For pandas.core.common you could have pandas.core import it and then replace it with a metamodule-style subclass without getting ctypes involved. |
More generally, what do people think about having an explicit contract for what's in the public vs private API? |
👍 I don't know if it's stated explicitly, but everything in |
any comments |
Tried to do a github search for where pandas.core.common was used, but it's mostly just a bunch of people's virtualenvs with pandas. Did find
No uses of pandas.core.common in seaborn or statsmodels, so that's good. |
yeah I added back the public stuff anyhow (though we should maybe use the meta-module tricks to deprecate it in 0.19.0). |
There's some tooling here to work around this obnoxious feature of GH search, in case you find it useful: https://github.com/njsmith/codetrawl/ |
Ok this was pretty easy to do
|
I agree with @jorisvandenbossche that it would be cleaner to simply have |
To not let hold up this PR, maybe we should move this discussion to another issue? (or discuss here, but regard it as independent of merging the PR) As the |
yes, and I think we SHOULD do this, and not just assume EVERYTHING outside is API, that's non-controllable. Merging on pass (had to fix some deprecations). |
warnings.warn("pandas.core.common.{t} is deprecated. " | ||
"import from the public API: " | ||
"pandas.api.types.{t} instead".format(t=t), | ||
FutureWarning, stacklevel=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback I think in this case case we should deviate from our normal rule of using FutureWarning for deprecations, and actually use a DeprecationWarning.
Typically, if people have used this, it will rather be in a package than in interactive explorative coding. And I think it is not needed that users of those packages see this warning (as they can't change it themselves)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But can be in a follow-up PR if you want to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh so this wont' show in downstream, ok, then. Though this does make it less visible. Let's decide in the followup issue.
ENH: add pandas.api Closes pandas-dev#12503
closes pandas-dev#12503 Author: Jeff Reback <jeff@reback.net> Closes pandas-dev#13147 from jreback/types and squashes the following commits: 244649a [Jeff Reback] CLN: reorg type inference & introspection
Deprecated back in v0.19.0 xref pandas-devgh-13147
Deprecated back in v0.19.0 xref pandas-devgh-13147
Deprecated back in v0.19.0 xref pandas-devgh-13147
Deprecated back in v0.19.0 xref pandas-devgh-13147
Deprecated back in v0.19.0 xref gh-13147
closes #12503
So this is the big boy. destroyed
core/common.py
and split up. Now have no more import issues and a nice private API. Nothing is exported by default. You have to explicity import things.There is now a
pandas.api
, so:Previously
pandas.core.common
was a huge namespace of everything under the sun. As a result, you often had to docom.
imports to do stuff, and sometimes import inside a function.These are essentially hierarchically arrange in order of deps.
pandas.types
generic
:ABC*
class infererence, no depsdtypes
: extension dtypes, no depsinference
: scalar / andis_list_list
instrospection, no depscommon
: imports all frominference
(for convenience), adds allis_*_dtype
things. deps on dtypes, generic, inferencemissing
:isnull
,notnull
,array_equivalent
: deps on commoncast
: the oldcore.convert
and all types of inference / conversions that littered core.common, deps on common. Mostly this is private stuff that Series/DataFrame uses (e.g. most methods are_
leading)pandas.core.common
now doesn't depend on anything else.So now can easily write:
from pandas.types.common import is_integer
or whatever at the top of any file and it will just work. Further almost all references tocom
gone.This only change was I added
array_equivalent
to thepd.
namespace, as formerly it was accessible as by:There should be NO user facing changes at all, but I will put a note that warns if people were using the 'private' API (IOW if they were directly importing stuff), then things have moved.
@wesm @jorisvandenbossche @shoyer @TomAugspurger @sinhrks