Skip to content
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: interleave columns pandas compat #15383

Conversation

raybellwaves
Copy link
Contributor

@raybellwaves raybellwaves commented Mar 25, 2024

Description

Add a pandas_compat note to DataFrame.interleave_columns

@raybellwaves raybellwaves requested a review from a team as a code owner March 25, 2024 03:41
Copy link

copy-pr-bot bot commented Mar 25, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Mar 25, 2024
@bdice bdice added doc Documentation non-breaking Non-breaking change labels Mar 25, 2024
@bdice
Copy link
Contributor

bdice commented Mar 25, 2024

/ok to test

@@ -4379,7 +4379,7 @@ def query(self, expr, local_dict=None):
1 2018-10-08

.. pandas-compat::
**DataFrame.query**
:func:`pandas.DataFrame.query`
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been just putting the API name in bold and not cross-linking it with intersphinx. I generally like using intersphinx but we'll have to see how it renders. If this change is accepted, would you be willing to make all the other .. pandas-compat:: sections match with an intersphinx reference in a second PR?

Copy link
Contributor Author

@raybellwaves raybellwaves Mar 25, 2024

Choose a reason for hiding this comment

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

If this change is accepted, would you be willing to make all the other .. pandas-compat:: sections match with an intersphinx reference in a second PR?

Should be ok.

Any idea why the docs build was skipped? https://github.com/rapidsai/cudf/actions/runs/8423601396/job/23067371447?pr=15383
I see a couple of checks failed below so it never made it to docs build
(https://github.com/rapidsai/cudf/actions/runs/8423601396/job/23066700203?pr=15383
conda_build.exceptions.DependencyNeedsBuildingError: Unsatisfiable dependencies for platform linux-aarch64: {MatchSpec("libkvikio=24.06"), MatchSpec("libcudf==24.06.00a39=cuda11_240325_gaf752c6cd0_39"), MatchSpec("rmm=24.06")}

https://github.com/rapidsai/cudf/actions/runs/8423601396/job/23066700960?pr=15383
conda_build.exceptions.DependencyNeedsBuildingError: Unsatisfiable dependencies for platform linux-aarch64: {MatchSpec("rmm=24.06"), MatchSpec("libcudf==24.06.00a39=cuda12_240325_gaf752c6cd0_39"), MatchSpec("libkvikio=24.06")}
)

@bdice
Copy link
Contributor

bdice commented Mar 26, 2024

We'll need #15392 to merge to 24.04, then forward-merge 24.04 to 24.06, then merge 24.06 into this PR before the docs will build correctly.

@raybellwaves raybellwaves marked this pull request as draft March 30, 2024 19:37
@raybellwaves
Copy link
Contributor Author

raybellwaves commented Mar 30, 2024

Had some issues with the pandas sphinx intermapping as well displaying a mapping in the pandas-compat directive (https://github.com/orgs/sphinx-doc/discussions/12212). I'll open back up if I make any progress

@vyasr
Copy link
Contributor

vyasr commented Apr 17, 2024

I'm holding off reviewing this until #15531 is in and we know that the pandas intersphinx mapping is working as expected. Happy to pick it back up again afterwards!

@raybellwaves
Copy link
Contributor Author

@vyasr no rush. I think i want to take a look at the pandas-compat sphinx extension first

rapids-bot bot pushed a commit that referenced this pull request Apr 18, 2024
This PR add pandas to intersphinx mapping to make it easy to link to pandas docs from the RAPIDS docs.

There is likely other opportunities to use the pandas intersphinx mapping e.g. #15383 but I think they can be subsequent PRs.

I've tested this locally and confirm it works as expected (i.e. the note in the docstring at https://docs.rapids.ai/api/cudf/stable/user_guide/api_docs/api/cudf.dataframe.query/#cudf.DataFrame.query is now hyperlinked to https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.query.html

![Screenshot 2024-04-14 at 2 15 33 AM](https://github.com/rapidsai/cudf/assets/17162724/193076e2-202e-4e74-9305-be1dbcdfa82b)

Apologies about the other linting. I can revert if need be

Authors:
  - Ray Bell (https://github.com/raybellwaves)
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Matthew Roeschke (https://github.com/mroeschke)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #15531
@vyasr
Copy link
Contributor

vyasr commented Apr 20, 2024

OK sounds good. Now that #15531 is merged feel free to ping again when you think this PR is ready for another look.

@raybellwaves raybellwaves marked this pull request as ready for review April 22, 2024 01:55
@raybellwaves
Copy link
Contributor Author

raybellwaves commented Apr 22, 2024

OK sounds good. Now that #15531 is merged feel free to ping again when you think this PR is ready for another look.

This is ready for review now.

@galipremsagar
Copy link
Contributor

/okay to test

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@wence-
Copy link
Contributor

wence- commented May 2, 2024

/merge

@rapids-bot rapids-bot bot merged commit 6882870 into rapidsai:branch-24.06 May 2, 2024
70 checks passed
rapids-bot bot pushed a commit that referenced this pull request Jun 10, 2024
This PR allows the PandasCompat sphinx ext to contain resolved references. For example, you can now add intersphinx mapping to the content of the admonition.

### Motivation

I enjoy connecting the PyData communities and this PR allows for more opportunities to use intersphinx mapping to link back to the pandas docs.

### History

I first tried this in a previous PR (#15383 (comment)) and commented here (#15383 (comment)) that I may get around to investigating this further. I finally had to time to work on this and made a bit of progress.

### Testing

I created a separate repo for this at https://github.com/raybellwaves/compatsphinxext which deploys straight to https://raybellwaves.github.io/compatsphinxext you can see it's working as expected here: https://raybellwaves.github.io/compatsphinxext/compat.html. You should be able to fork that and tinker pretty quickly.

### Further work

This could be cleaned up (for example I couldn't get the [source] to display in the admonition as I worked from the latest sphinx todo extension (https://github.com/sphinx-doc/sphinx/blob/master/sphinx/ext/todo.py)). The existing pandas-compat Admonition's could be switched to this if agreed. In addition, the documentation around how to write pandas-compat entries going forward (https://github.com/rapidsai/cudf/blob/branch-24.06/docs/cudf/source/developer_guide/documentation.md#comparing-to-pandas) will also have to be updated.

Longer term the extension could be published and used across RAPIDS libraries where there are differences in compatibility with PyData libraries e.g. pandas, network, scikit-learn to simplify linking to those dos. I'm not sure if I'll have time to work on this though.

Authors:
  - Ray Bell (https://github.com/raybellwaves)
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #15704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants