Skip to content

DOC: Show constructor arguments for some classes in pd.series.offsets #61605

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Dr-Irv
Copy link
Contributor

@Dr-Irv Dr-Irv commented Jun 8, 2025

This contributes to #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. It seems that if you have class that is a not a cdef class, you have to have a __new__() method so the docs show up. This PR does this for YearEnd, by creating a cdef _YearEnd class. and then YearEnd is a regular class with __new__() calling __new__() of _YearEnd. So, if you preview the docs from this MR, you will see that YearEnd now shows its constructor.

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 a Parameters section to those classes. Note that for YearEnd, I had to change the docs from Attributes to Parameters to make it so that the docstrings are validated.

f"\'{c_PERIOD_AND_OFFSET_DEPR_FREQSTR.get(name)}\'"
f" instead.",
f"\'{c_PERIOD_AND_OFFSET_DEPR_FREQSTR.get(name)}\' "
f"instead.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dr-Irv this and the following change have for some time caused pre-commit to fail for me locally.

great that this is being fixed here. Thanks.

Any idea why CI hasn't picked this up before now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was surprised as well.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jun 8, 2025

/preview

@datapythonista
Copy link
Member

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.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jun 9, 2025

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.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jun 22, 2025

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.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jun 22, 2025

/preview

1 similar comment
@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jun 23, 2025

/preview

Copy link
Contributor

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@rhshadrach
Copy link
Member

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 a Parameters section to those classes.

This seems like a very heavy handed code change for docs.

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jul 1, 2025

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

@Dr-Irv
Copy link
Contributor Author

Dr-Irv commented Jul 1, 2025

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 a Parameters section 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.

@rhshadrach
Copy link
Member

I believe this is cython/cython#3873.

@jbrockmendel - any thoughts here?

@jbrockmendel
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants