-
-
Notifications
You must be signed in to change notification settings - Fork 19.1k
DOC: Set __module__ attributes and remove core
from documentation
#62727
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
base: main
Are you sure you want to change the base?
Conversation
""" | ||
|
||
__module__ = "pandas" | ||
__module__ = "pandas.api.typing" |
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.
I get wanting NatType in the api,typing namespace, but do t we want pd.NaT in the pandas namespace? Same for pd.NA above
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.
Ack, I didn't realize we'd also need to consider __module__
on class instances. Thanks.
core
from documentation
core
from documentationcore
from documentation
pandas/tests/api/test_api.py
Outdated
or ("Dtype" in name and obj.__module__ == "pandas") | ||
or (name == "Categorical" and obj.__module__ == "pandas") | ||
# TODO: Can't seem to change __module__ | ||
or name == "Interval" |
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.
Modifying __module__
on Interval doesn't seem to have an impact; I'm guessing because this is a cdef class?
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.
@jbrockmendel - this is about ready, just this last todo I think. Was wondering if you could have a look here.
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.
I don’t have any good ideas here.
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.
Looks like for Period
, Timestamp
, Timedelta
, there is this in there:
@set_module("pandas")
class Period(PeriodMixin):
So maybe that's all you need to do?
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.
Those classes aren’t cdef
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.
Right. This relates to the doc issue for Interval
that is the same as what happened with the offsets. Maybe we should make the cdef class _Interval
and then just have the python class Interval
with all the docs, etc., and have Interval
be a subclass of _Interval
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.
I don't think that'd be worth it just to set the __module__
attribute, I am okay here as-is.
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.
Resolved via cython/cython#7231 (comment)
__module__
on top-level public objects #55178 (Replace xxxx with the GitHub issue number)doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.This PR addresses all
__module__
attributes of public objects that I can find with the exception of functions. It's not clear to me if we want to also be modifying the__module__
attribute of e.g.pandas.factorize
and even if we do, I'd like to keep that separate as this PR is already becoming sizable.Remaining documentation that contains
.core.
:API docs
User Guide
Getting Started
no output.