Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

DOC: allow jax to correctly reference numpy documentation #22162

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

Closed
wants to merge 1 commit into from

Conversation

ilemhadri
Copy link

@ilemhadri ilemhadri commented Aug 22, 2022

@jakevdp pointed out this might be necessary in order to get jax documentation links pointing to numpy's to correctly parse the numpy hyper-links.

The discussion that triggered this comment started here

@rgommers WDYT?

@ilemhadri ilemhadri changed the title DOC: allow numpy-like modules to rely on numpy documentation DOC: allow jax to correctly reference numpy documentation Aug 22, 2022
@mattip
Copy link
Member

mattip commented Aug 22, 2022

Are there more pages that need the directive?

@mattip
Copy link
Member

mattip commented Aug 22, 2022

LGTM.

For the curious, this PR does not change the number of warnings in our pedantic mode doc build, both before and after there are 183 warnings about "reference not found"

@jakevdp
Copy link
Contributor

jakevdp commented Aug 22, 2022

Just a note, since I was credited with this: I'm not certain this is the correct fix.

What I am certain of is that certain ufuncs in the NumPy documentation currently do not correctly generate cross-reference hyperlinks. For example, if cross-links were generated correctly, I believe every entry in the third column of this table in NumPy's docs would have a hyperlink, but for some reason only divmod does.

We see the same thing in JAX when we use intersphinx to attempt to generate cross-links to these function, but note that this is an issue in NumPy even without any reference to the JAX project.

I haven't checked whether the suggested fix here fixes the issue, but I came up with it as a possible culprit after a few minutes of browsing the doc source and trying to figure out what may have been different between divmod and the other functions in that table.

@seberg
Copy link
Member

seberg commented Aug 22, 2022

@rossbar if you think this is probably right, lets give it a shot?

@rossbar
Copy link
Contributor

rossbar commented Aug 22, 2022

According to the objects.inv, the ufunc link targets are listed under the :py:data: role, not :func:. I went ahead and tried switching the roles from :func: to :py:data: in NEP 13 (see @jakevdp 's comment above) and sure enough it fixes the links (note that the divmod link that "works" actually points to the Python builtin divmod, not numpy's).

I do think it would be an improvement if the generated ufunc stubs were recognized by the :func: role, but I'm not sure exactly why/how this works without taking a deeper look. I'm not sure that this change will fix the problem ID'ed in jax-ml/jax#11578. FWIW I tried the following procedure to see if this would fix the problem in NEP 13:

  1. Added a row to the table in NEP 13 for the cos function
  2. Update the intersphinx mapping in the NEPS conf.py to point to the objects.inv from the circleCI artifacts in this PR
  3. Built the NEPS

The link to cos was still not working despite the updated objects.inv. @ilemhadri can you confirm that this patch fixes the problem for you? If so I'd be fine with adding it (even though I don't fully understand how it fixes the link issue).

Alternatively, if it's easy for you to update the role which is used to link the numpy ufuncs in the jax docs to :py:data:, that might be another solution.

@ilemhadri
Copy link
Author

ilemhadri commented Sep 20, 2022

@ilemhadri can you confirm that this patch fixes the problem for you?

I don't have a way of testing this locally, but @jakevdp might be able to check!

@seberg
Copy link
Member

seberg commented Oct 12, 2022

@rossbar is there a point in doing this whether it helps or not? In that case, maybe we should just merge, otherwise maybe simply close (can always reopen/open a new PR)?

@ilemhadri
Copy link
Author

@seberg I would vote in favor of just merging!

@seberg
Copy link
Member

seberg commented Jan 19, 2023

It seems the fun trick to make it py:function role would be do the same as in gh-23039. I wonder how much doc linking that would break, to "fix" it to that role, though?

@seberg
Copy link
Member

seberg commented Jan 20, 2023

Going to close the PR, nobody seems sure that this makes any difference. The role of ufuncs is currently data, which could be changed (see gh-23039). So if JAX uses :func: it can't work to begin with.

I am happy to change the role :data: as said above, @rossbar what do you think?

@seberg seberg closed this Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.