GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — sphinx-codeautolink improvements by scottyhq · Pull Request #281 · xarray-contrib/xarray-tutorial

Conversation

scottyhq commented Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Based on discussion in #82

adds underlines to linked code docs
does some mapping to xarray api

Lots of warnings! Would need to add lots of manual mappings for completeness... and even then it doesn't pick up things like da.resample and da.isel below because it doesn't realize that da is a DataAarray:

da = xr.tutorial.load_dataset("air_temperature", engine="netcdf4").air
monthly = da.resample(time="ME").mean()
data = da.isel(time=0)

Even so, I think the underline and hover color are nice additions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

scottyhq added 2 commits

June 30, 2024 22:54


          autolink improvements

ad64161


          warnings dont count as errors

100e767

github-actions Bot commented Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

🎊 PR Preview 2e3e0f5 has been successfully built and deployed to https://xarray-contrib-xarray-tutorial-preview-pr-281.surge.sh

🕐 Build time: 0.01s

_{🤖 By surge-preview}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          more mappings, add typing to code examples

2e3e0f5

review-notebook-app Bot commented Jul 1, 2024

Copy link

Copy Markdown

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

scottyhq commented

Jul 1, 2024

View reviewed changes

Comment thread

_config.yml

Comment on lines +81 to +87

		codeautolink_inventory_map: {
		# unfortunately mapping the top level doesn't work, need all methods
		#"xarray.core.dataarray.DataArray": "xarray.DataArray",
		"xarray.core.accessor_dt.DatetimeAccessor": "xarray.DataArray.dt",
		"xarray.core.dataarray.DataArray.coarsen": "xarray.DataArray.coarsen",
		"xarray.core.dataarray.DataArray.groupby": "xarray.DataArray.groupby",
		"xarray.core.dataarray.DataArray.groupby_bins": "xarray.DataArray.groupby_bins",

scottyhq Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@felix-hilden, I realize this issue (felix-hilden/sphinx-codeautolink#131 (comment) ) has been closed for a while, but curious based on your comment if it does seems straightforward to implement a simplification of ("xarray.core.dataarray.DataArray": "xarray.DataArray") to avoid having to enumerate every method?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dcherian Jul 1, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Would setting __module__ fix this? (pydata/xarray#4279)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

scottyhq reacted with eyes emoji

All reactions

👀 1 reaction

scottyhq Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Based on the above issue, if I add

import xarray 
xarray.DataArray.__module__ = "xarray"

to conf.py before building I'm getting Handler <bound method SphinxCodeAutoLink.create_references of <sphinx_codeautolink.extension.SphinxCodeAutoLink object at 0x145730710>> for event 'env-updated' threw an exception (exception: maximum recursion depth exceeded) ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dcherian Jul 1, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you try adding it to xarrays init.py?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

felix-hilden Jul 1, 2024

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hi! Yeah I'm not at all opposed to having the feature that was discussed in the issue, I've just been out of the game for a while focusing on other things and I have personally no need for that. But I'm very open to contributions 👌

Personally I've opted for changing module attributes like so and it's worked well for me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

scottyhq reacted with thumbs up emoji

All reactions

👍 1 reaction

scottyhq Jul 1, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

actually it does look like chaning either conf.py or xarray/__init__.py works, I think the recursion error was coming from some issue with my local settings/cache...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

scottyhq Jul 1, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

kind of a pain to modify conf.py via jupyter book (jupyter-book/jupyter-book#858,
jupyter-book/jupyter-book#1673 (comment)) but that is probably the way to go...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dcherian Jul 1, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Let's do it upstream? I think we'd like this in Xarray too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

scottyhq commented

Jul 1, 2024

View reviewed changes

Comment thread

_static/style.css

Comment on lines +5 to +15

		.sphinx-codeautolink-a:link {
		border-bottom-color: lightgray;
		border-bottom-style: dotted;
		border-bottom-width: 1px;
		}

		.sphinx-codeautolink-a:hover {
		border-bottom-color: rgb(255, 139, 139);
		border-bottom-style: dotted;
		border-bottom-width: 1px;
		}

scottyhq Jul 1, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@dcherian @zmoon I think a subtle dotted underline is nice, personally I think the thick underlines (https://scikit-image.org/docs/stable/auto_examples/segmentation/plot_regionprops.html) are a bit distracting and distinct from the jupyterlab experience, but open to other opinions!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zmoon Jul 1, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I concur!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dcherian Jul 1, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A little darker would be nicer to me. It's also convention that solid lines are clickable links, so perhaps just a thin darker line?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

scottyhq reacted with thumbs up emoji

All reactions

👍 1 reaction

scottyhq commented

Jul 1, 2024

View reviewed changes

Comment thread

intermediate/01-high-level-computation-patterns.ipynb

		},
		"outputs": [],
		"source": [
		"da: xr.DataArray = xr.tutorial.load_dataset(\"air_temperature\", engine=\"netcdf4\").air\n",

scottyhq Jul 1, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Need a type hint on da in order for subsequent methods to be picked up (da.resample). Also, if chaining methods only the first is picked up da.isel(time=0).plot() links to isel but not plot

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

scottyhq Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Opened discussion for clarification on this felix-hilden/sphinx-codeautolink#146.

Edit: method chaining links would need some work felix-hilden/sphinx-codeautolink#147

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

scottyhq marked this pull request as draft

May 29, 2025 14:43

dcherian mentioned this pull request

Oct 6, 2026

Add inventory aliases for class members at their definition path pydata/xarray#11678

Merged

1 task done

This branch has not been deployed

No deployments

Reviewers

dcherian

dcherian left review comments

+2 more reviewers

zmoon

zmoon left review comments

felix-hilden

felix-hilden left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

scottyhq commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

review-notebook-app Bot commented Jul 1, 2024

Uh oh!

scottyhq Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottyhq Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottyhq Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scottyhq commented Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

scottyhq Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

scottyhq Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

scottyhq Jul 1, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.