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

Add cross-links to API docstring from tutorial and user-guide #8311

Merged
merged 9 commits into from
Nov 5, 2023

Conversation

amanbagrecha
Copy link
Contributor

@amanbagrecha amanbagrecha commented Oct 15, 2023

Cross links for

  • apply_ufunc
  • weighted
  • indexing
  • interp
  • map_blocks
  • groupby
  • coarsen

@welcome
Copy link

welcome bot commented Oct 15, 2023

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

xarray/core/dataarray.py Outdated Show resolved Hide resolved
xarray/core/dataset.py Outdated Show resolved Hide resolved
xarray/core/dataset.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

Thanks @amanbagrecha . I changed the links to more general pages.

@amanbagrecha
Copy link
Contributor Author

Thanks @dcherian for the edits. I reverted the docs you made for apply_ufunc to the correct url. Also added additional cross-links to user-guide and tutorials. I have few questions

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

I wonder if we should be using the intersphinx entries in https://tutorial.xarray.dev/objects.inv?

If so, you'd need to add

    "xarray-tutorial": ("https://tutorial.xarray.dev/", None),

to intersphinx_mapping

doc/conf.py Outdated Show resolved Hide resolved
@@ -6788,6 +6801,13 @@ def weighted(self, weights: DataArray) -> DataArrayWeighted:
See Also
--------
Dataset.weighted

`Weighted array reductions <https://docs.xarray.dev/en/stable/user-guide/computation.html#weighted-array-reductions>`_
Copy link
Collaborator

@keewis keewis Oct 22, 2023

Choose a reason for hiding this comment

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

you can use sphobjinv su -u https://docs.xarray.dev/en/latest computation to find the appropriate link, in this case

:std:label:`/user-guide/computation.rst#weighted-array-reductions`

(though maybe we need to add label directives to user-guide/computation to make this easier? E.g. add

.. _weighted-array-reductions:

just before that section... not sure if - is a allowed character in labels, though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

many thanks! I made the necessary changes.

@amanbagrecha
Copy link
Contributor Author

I wonder if we should be using the intersphinx entries in https://tutorial.xarray.dev/objects.inv?

If so, you'd need to add

    "xarray-tutorial": ("https://tutorial.xarray.dev/", None),

to intersphinx_mapping

I added xarray-tutorial to intersphinx_mapping. Though I am confused how to use it. I tried sphobjinv su -u https://tutorial.xarray.dev/ fundamentals and found the necessary link, though unable to reference it correctly.
I tried the following to no avail. How do I resolve this?

:doc:`fundamentals/03.4_weighted`

@keewis
Copy link
Collaborator

keewis commented Oct 22, 2023

not sure, you might have to use a prefix:

:doc:`xarray-tutorial:fundamentals/03.4_weighted`

where the prefix is the value you put as the dict key in intersphinx_mapping

@amanbagrecha
Copy link
Contributor Author

not sure, you might have to use a prefix:

:doc:`xarray-tutorial:fundamentals/03.4_weighted`

where the prefix is the value you put as the dict key in intersphinx_mapping

Thanks this worked. Now I'm struggling to find a way to link just the section as in https://tutorial.xarray.dev/intermediate/01-high-level-computation-patterns.html#label-space-windows-or-bins-groupby. Since tutorials are in markdown, I cannot add a directive I think.

@keewis
Copy link
Collaborator

keewis commented Oct 22, 2023

I think this should be possible (not sure though, especially given jupyter-book/jupyter-book#1767), but might need changes to the tutorial. @dcherian, any ideas?

@dcherian
Copy link
Contributor

No ideas but this is a major improvement. So let's just go ahead with a link to the whole page.

@keewis
Copy link
Collaborator

keewis commented Nov 2, 2023

was there anything else you'd like to change, @amanbagrecha? Otherwise I think we can undo the nbsphinx error setting change and merge.

@amanbagrecha amanbagrecha marked this pull request as ready for review November 2, 2023 17:36
@dcherian dcherian requested a review from keewis November 2, 2023 20:58
Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

we might have to revisit the links once xarray-tutorial has section targets, but for now this looks good to me. Thanks a lot, @amanbagrecha

@@ -1431,6 +1431,12 @@ def isel(
Dataset.isel
DataArray.sel

:doc:`xarray-tutorial:intermediate/indexing/indexing`
Copy link
Contributor

Choose a reason for hiding this comment

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

:( This seems to have messed up some formatting though:
image

while on main
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be valid, though. So we might need to fix napoleon to allow this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it appears we had a similar issue more than two years ago when we first introduced :doc: links to See Also (see #5176), where we concluded that this is a bug in napoleon (nobody got around to reporting / fixing that since then, though).

Copy link
Contributor

Choose a reason for hiding this comment

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

OK well this is an improvement in any case. Merging.

@dcherian
Copy link
Contributor

dcherian commented Nov 5, 2023

Thanks @amanbagrecha this is a great contribution. Welcome to Xarray!

@dcherian dcherian merged commit 31c6e62 into pydata:main Nov 5, 2023
28 checks passed
Copy link

welcome bot commented Nov 5, 2023

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

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

Successfully merging this pull request may close these issues.

4 participants