-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
DOC: set __module__ on remaining top-level functions #62758
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
DOC: set __module__ on remaining top-level functions #62758
Conversation
@jorisvandenbossche - I've addressed the remaining functions. |
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.
lgtm (assuming green)
We get a doctest failure for one of the plotting functions that are touched:
Which is not happening on main, and I also don't see any change in the environment. But I also don't directly see how this change is related .. |
@jorisvandenbossche - this happens when we are ignoring some validation check. Need to update the path to the new one that is used in the model attribute. I can take a look later today. |
Ah yes. But, I also don't directly see any skips for doctests for plotting in general or lag_plot specifically? |
Indeed, this is very weird. Setting the I'm guessing there is something going odd with either pytest or matplotlib setup. The changing of the repr seems harmless and doesn't happen during normal runtime. Maybe just update the docstring? |
Alright, this is on our end. The only thing Reproducer (whether the data = {
"length": [1.5, 0.5, 1.2, 0.9, 3],
"width": [0.7, 0.2, 0.15, 0.2, 1.1],
}
index = ["pig", "rabbit", "duck", "chicken", "horse"]
df = pd.DataFrame(data, index=index)
hist = df.hist(bins=3)
np.random.seed(5)
x = np.cumsum(np.random.normal(loc=1, scale=5, size=50))
s = pd.Series(x)
print(repr(pd.plotting.lag_plot(s, lag=1)))
# <Axes: title={'center': 'width'}, xlabel='y(t)', ylabel='y(t + 1)'> |
A bit more clarity: it's the doctest of |
@jorisvandenbossche - fingers crossed this should be good. The issue was that |
pandas/conftest.py
Outdated
@pytest.fixture(autouse=True) | ||
def mpl_cleanup(doctest_namespace): |
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.
Could we just change that matplotlib example instead (or add cleanup code in the doctest?). I don't think this fixture in particularly cheap to run for every test
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.
Ah, I was thinking with doctest_namespace
this would only be autoused for doctests. Thanks for catching this. One alternative is to have mpl_cleanup_doctest
that is set to autouse=True
with the first lines:
if not isinstance(request.node, pytest.DoctestItem):
return
This would at least make it cheap (~95ns on my machine), though still called for every test. And we'd still need mpl_cleanup
that doesn't have autouse=True
.
But I'm more in favor of reverting this and just supressing the output for this one test. It isn't great that one doctest can impact another (took a few hours to figure out what was going on here), but apparently is quite rare for it to have any negative side effects.
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 I'm more in favor of reverting this and just suppressing the output for this one test
Yes I would be in favor of this too.
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.
An alternative could also be to put (a copy of) that fixture in a conftest.py file in pandas/plotting directory, so it will (I think?) only be auto-used for tests (and thus only doctests) in that directory?
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 like that @jorisvandenbossche - a few things come to mind.
- This would not cover all docstrings with plotting, but I believe the majority of them.
pandas.plotting
is public, so maybe we move_core.py
and_misc.py
into a_base
(or some other name) sudirectory. This way theconftest
submodule doesn't appear as public.- Would prefer adding
mpl_cleanup
topandas._testing
and then calling this function from the two fixtures.
return InteractiveShell(config=c) | ||
|
||
|
||
@pytest.fixture |
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 this was accidentally deleted
@rhshadrach thanks a lot for diving in the doctest failure .. ! Going to merge now as is, I think we should still consider using the fixture (as you mentioned, it is a very nasty issue to debug if it happens, even though that will be rare, if we have a way to prevent that time spent in the future) |
Follow-up on #62727, resolving this for the remaining functions in the top-level namespace (and for some of the submodules, where it were only a few ones to address, i.e. pandas.testing and pandas.api.extensions)
xref #55178