Add sphinx-autocodelink & intersphinx_mapping

[dcherian]

xref #82

This somewhat works.

• intersphinx to xarray doesn't work
• sphinx-codeautolink seems buggy and doesn't work in as many places it could (e.g. Consider linking assignment targets, arguments and single names felix-hilden/sphinx-codeautolink#109)

[zmoon]

• intersphinx to xarray doesn't work

Just curious about this. Like {py:class}`xarray.Dataset` / {external+xarray:py:class}`xarray.Dataset` doesn't work? Or using [title](ref)? Does it work with the pydata docs URL?

Update: I tried it out and both of those syntaxes work for me so I assume it is the codeautolink that you were referring to

[zmoon]

After a bit of investigation, seems like xarray isn't working because it doesn't have an xarray module doc reference? e.g. for NumPy 'numpy': 'https://numpy.org/doc/stable/reference/index.html#module-numpy' is in the inventory that it looks in to resolve the imports. And Matplotlib https://matplotlib.org/stable/index.html#module-matplotlib, with .. module:: matplotlib on the home page here.

Consistent with this, {mod}`xarray` doesn't work either (although e.g. {mod}`numpy` does). dask doesn't have one either.

So, perhaps adding Sphinx module directive for the xarray module in the xarray docs would fix it.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[scottyhq]

this is awesome. @dcherian you'll have to re-render the lockfile with the new pip package b/c that is what gets used in CI https://github.com/xarray-contrib/xarray-tutorial/tree/main/conda#createupdate-multiplatform-lockfile

[dcherian]

Thanks for the conda lock tip.

I'm 50/50 on this PR. When it works, it's great. But I find myself hovering over things that just don't work. Like

ds = xr.Dataset(...)

will not link Dataset!

xr.Dataset

^ This supposedly will link though

[scottyhq]

gotcha. I'm fine leaving this in draft form and revisiting at some later date, or separate out just the intersphinx piece (maybe just for xarray?) and revisit the code block autolinking later?

[zmoon]

When it works, it's great

When I was trying it I found that it only seemed to work for full build from clean, not partial builds, which is kind of annoying.

[felix-hilden]

Yooo thanks for trying autolink out, author here 👋 Did you encounter other bugs besides the one mentioned?

ds = xr.DataSet(...)

should be linked in terms of the xr.DataSet part, but indeed felix-hilden/sphinx-codeautolink#109 will perhaps address the assignment target! Notebooks have been a pain point though, I wouldn't be surprised if there are things to iron out. Much appreciated!

[dcherian]

The following seems like it should be supported or is it felix-hilden/sphinx-codeautolink#65?

Here da is a DataArray

So i'd expect

1. da to link to https://docs.xarray.dev/en/latest/generated/xarray.DataArray.html
2. .resample to link to https://docs.xarray.dev/en/latest/generated/xarray.DataArray.resample.html, (similarly for .isel, .plot)
3. and the .mean to https://docs.xarray.dev/en/latest/generated/xarray.core.resample.DataArrayResample.mean.html

https://sphinx-codeautolink.readthedocs.io/en/latest/examples.html#concatenating-examples and https://sphinx-codeautolink.readthedocs.io/en/latest/examples.html#concatenating-examples shows the behaviour I want.

[felix-hilden]

Yep, it should work as long as the type hints are available 👌 Getting air might be the culprit here if it's not working. At a quick glance I didn't find it in your code / documentation.

[dcherian]

It's the first cell in https://github.com/xarray-contrib/xarray-tutorial/blob/8f250e358af50998174ad08f311069bb044421d1/intermediate/01-high-level-computation-patterns.ipynb

[felix-hilden]

Right, but the reference documentation or type hinted importable location of "air" I meant!

[dcherian]

Oh I see the problem now. It comes from this mixin class but the return value is Any so that's useless for linking I guess.

https://github.com/pydata/xarray/blob/d0bfd00d41fd3a70b70f7f63b7f94f775da9be9c/xarray/core/common.py#L271

I can change the code to use Dataset.__getitem__ instead.

[felix-hilden]

Yep most likely. Fair warning though, I don't think we provide any special support for __getitem__. You might be better off type hinting the variable instead. We override the target with type hints (test here although it's a bit hard to read).

[dcherian]

Still no luck after explicit typing unfortunately

[felix-hilden]

Oh, that's unfortunate indeed. resample should definitely be available judging by the documentation. I'll debug this later this week and get back to you.

[felix-hilden]

I found the culprit! The inventory and documentation entry for DataArray is xarray.DataArray, and rightly so of course since it's much more user-friendly than using the full import. The problem is that we use the fully qualified name of a class to determine the canonical location of a class to link items. This is done to avoid issues when bouncing around the library imports when resolving names to locations.

So linking DataArray correctly would require us to edit the module location by hand, much like I've done here. Maybe you don't want to go as far as to mark the underlying modules as private, but just modifying the attribute should suffice.

[dcherian]

Oh interesting. Thanks for tracking that down.

Would it be feasible to do this as a variable in conf.py? Say

codeautolink_module_overrides = {
	"DataArray": "xarray",
	"Dataset": "xarray",
}

[felix-hilden]

I like the idea! Let's continue the discussion on the linked issue.

[github-actions]

♻️ PR Preview 8df1193 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

[scottyhq]

@dcherian I wanted to revive this one now that I have a previews working again and bumped all the package versions.

To get methods like da.resample() and others to link correctly (e.g. https://xarray-contrib-xarray-tutorial-preview-pr-85.surge.sh/intermediate/01-high-level-computation-patterns.html). It sounds like we'd need to add a custom map file to this repository based on felix-hilden/sphinx-codeautolink#131 (comment)

So,,, options are 1: merge as-is with limited functionality (I'm happy with this), 2: add a custom map file, or 3: abandon adding this feature

[dcherian]

Merge away!