pandas-dev
DOC: Show constructor arguments for some classes in `pd.series.offsets`
This contributes to https://github.com/pandas-dev/pandas/issues/52431
In that issue, the goal is to show the arguments for the constructors for the various offsets, which are all in pyx files.
The problem is that sphinx doesn't pick up the arguments for __init__() in those files. By adding the # cython: embedsignature=True at the top of the file, then it will pick it up for any class we document that has an explicit __init__() method. So, if you preview the docs from this MR, you will see it for classes like YearEnd, and BusinessHour because they have explicit __init__() methods. But you won't see it for YearBegin, for example, because it has no explicit __init__() method.
So to fix that, I illustrate how to fix it for Day, where I introduce an __init__() method that just calls the super().__init__(), and that makes sphinx then pick up the docs for pandas.tseries.offsets.Day
Also, I had to add the okexcept things to doc/source/user_guide/enhancingperf.rst because otherwise the docs would not build on Windows.
If this PR is accepted, then we can get the community to do the work of adding the various __init__() methods to the other documented offset classes and they can also add a Parameters section to those classes, which is what I did for Day as well.
No strong opinions about this, but if the examples need the okexcept on windows sounds like this change is breaking things on windows, no? I guess I'm missing something, if you can clarify. But it would be better to fix the underlying problem, I don't think we should add okexcept unless we document exceptions.
No strong opinions about this, but if the examples need the okexcept on windows sounds like this change is breaking things on windows, no? I guess I'm missing something, if you can clarify. But it would be better to fix the underlying problem, I don't think we should add okexcept unless we document exceptions.
No, the change is needed to just build the regular documentation (without this change) on Windows. It has something to do with how sphinx calls cython and that doesn't seem to work right on Windows. Not sure exactly what the problem is.
No strong opinions about this, but if the examples need the okexcept on windows sounds like this change is breaking things on windows, no? I guess I'm missing something, if you can clarify. But it would be better to fix the underlying problem, I don't think we should add okexcept unless we document exceptions.
I found a better fix in #61686 . So the okexcept is not there any more.
Website preview of this PR available at: https://pandas.pydata.org/preview/pandas-dev/pandas/61605/
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
If this PR is accepted, then we can get the community to do the work of separating the classes into "public" and "private" ones and adding adding the
__new__()methods to the other documented offset classes and they can also add aParameterssection to those classes.
This seems like a very heavy handed code change for docs.
From the linked issue:
It turns out that making the constructor arguments appear from PYX files that are imported 3 levels in the hierarchy doesn't work right with Sphinx.
Has this been reported?
It was a problem in our repo. It is fixed in this PR in doc/source/index.rst.template
If this PR is accepted, then we can get the community to do the work of separating the classes into "public" and "private" ones and adding adding the
__new__()methods to the other documented offset classes and they can also add aParameterssection to those classes.This seems like a very heavy handed code change for docs.
No disagreement there. But I don't see another way of getting these constructors documented unless we work out something with the sphinx developers.
Note that there is a similar problem for Interval (https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.Interval.html#pandas.Interval) . For Timestamp, Timedelta, and Period, the same pattern is used so that the constructors show up in the docs, i.e., there is a private cdef class and the public class is a subclass of the private one, and the public one has all the docs.
I believe this is https://github.com/cython/cython/issues/3873.
@jbrockmendel - any thoughts here?
This came up in the last dev call. I think Irv's solution was to make a non-cdef class with the docstring. The perf impact was about 100ns on construction which we agreed was not a big deal (just kind of janky).
For Timestamp, Timedelta, and Period, the same pattern is used so that the constructors show up in the docs
FWIW im pretty sure that pattern is used because we implement a __new__ method, which I don't think you can do in a cdef class.
Is this something that can be reverted once sphinx/cython fixes something upstream? if so, can there be a # TODO(sphinx3.14.159): ... attached to these
Is this something that can be reverted once sphinx/cython fixes something upstream? if so, can there be a
# TODO(sphinx3.14.159): ...attached to these
Maybe. I don't know how easy the reversion would be. But if we make this change, we can also better document all the offset methods/properties and avoid the exceptions that appear in https://github.com/pandas-dev/pandas/blob/faf3bbb1d7831f7db8fc72b36f3e83e7179bb3f9/ci/code_checks.sh#L81
@rhshadrach at the dev meeting on 8/13, @jbrockmendel said he was fine with this solution. Just need you to agree....