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

Fix broken references #605

Closed
twiecki opened this issue Nov 18, 2023 · 7 comments
Closed

Fix broken references #605

twiecki opened this issue Nov 18, 2023 · 7 comments
Labels
good first issue Good for newcomers

Comments

@twiecki
Copy link
Member

twiecki commented Nov 18, 2023

Various NBs have broken references. Mainly within the two following categories:

Theano-PyTensor references

There are still several links to deeplearning.com from the old theano docs. They should be converted to sphinx cross references instead of raw URLs and have them point to the closes page in the pytensor documentation (pytensor has also had many changes since theano so not all pages exist anymore).

Example
In the blackbox log likelihood with numpy, the last paragraph of the introduction has two hyperlinks:

none of which make sense anymore and deeplearning.net. Moreover, due to the multiple find and replace the repository has gone through, those links wouldn't even point anywhere anymore. The following steps might help through the process:

  1. Try to see if that page still exists in pytensor docs (though most of the time that is not the case anymore). To do that, replace the http://deeplearning.net/software/pytensor/ with https://pytensor.readthedocs.io/en/latest/.

    In this particular case, the first no longer exist, but the second does. Our target URL is https://pytensor.readthedocs.io/en/latest/extending/op.html#grad. So for that link we can jump to step 4

  2. Try to find the original page to see what it was about. This will help you find its closest equivalent in the current pytensor docs. Go to https://github.com/Theano/Theano/tree/master/doc and then browse to the path that is left after removing the http://deeplearning.net/software/pytensor/ part of the url.

    In this particular case, we have to go into the extending folder and look at the extending_theano file in there (remember the multiple find and replaces!). The title of that file is "Creating a new Op: Python implementation". If we look at the pages of the extending section in pytensor docs (see the sidebar) we see there is a page with that same name. But the page has been renamed: https://pytensor.readthedocs.io/en/latest/extending/creating_an_op.html.

  3. If none of the above work, try finding a page that matches what the url text+context imply has to be found there and/or ask for help in an issue or after opening a draft PR.

  4. Once you have your target URL, see how to express it as a sphinx cross-reference. There are three options:

    • The target is a python object. This is the case with the grad method link. In such case, we have to find its full import path and its type. Then use {type_abbr}`import.path`. To have the link show only the the object and skip the import path, add a ~ as start of the import path. See this page for more examples and the type abbreviation reference.

      The grad method example falls here. It is a function so we should use {func}`grad`. Note it isn't always clear to see how an object is documented (and we don't always follow best practices); don't hesitate to ask.

    • The target is a page with a sphinx anchor. Sphinx anchors look like .. _anchor: or (anchor)=. You can see if one exists using the "show source" button on the right sidebar of all pages. Then use {ref}`anchor` (url text will be the page/section heading the anchor is at) or {ref}custom text ` `` to use custom text.

      The first link falls here. If we click on "show source" in the creating an op page we will see it starts with .. _creating_an_op:. So we should use {ref}`PyTensor Op <creating_an_op>`. Note the _ is part of the syntax, not of the anchor.

Internal cross references

There are now multiple internal references that are broken all around. With the recent restructuring and removing of the outdated notebooks, existing targets and pages don't exist anymore.

These should all be {ref} `` type cross-references to other notebooks. In this case, you should see if there is any other similar page they could be "redirected" to, or if they should instead be deleted completely.

Important: These have to be checked from the notebook itself, not its rendered version.

Example
The same blackbox likelihood with numpy starts with

imatge

Here from the rendered version we can see the text doesn't make much sense, but when we look at the source, we see it has a {ref}`this other one <blackbox_external_likelihood>` which isn't rendered as a hyperlink. In this particular case, the whole "note" box should be deleted.

@OriolAbril
Copy link
Member

Updated the description to be more specific and will also pin the issue on the repo

@OriolAbril OriolAbril pinned this issue Nov 23, 2023
@itsdivya1309
Copy link
Contributor

I am new to open source and would like to work on this issue.

@OriolAbril
Copy link
Member

That would be great, thanks! Comment here which notebook you'll be working on to make sure nobody duplicates the work before you open the PR, but feel free to chose any notebook.

If you find notebooks with no issues also comment here so we know they have been proofread

@itsdivya1309
Copy link
Contributor

Hey!
I skimmed through the notebooks, and I would like to work on the notebooks in the 'How to' section.

@twiecki
Copy link
Member Author

twiecki commented Nov 28, 2023

@itsdivya1309 Great - just open a PR!

@itsdivya1309 itsdivya1309 mentioned this issue Dec 3, 2023
3 tasks
@itsdivya1309
Copy link
Contributor

I have updated the NB Profiling.
But I also updated it to version 5.10 (it was written in v3.9.3). Should I open another PR for this?

@twiecki
Copy link
Member Author

twiecki commented Jan 21, 2024

I have updated the NB Profiling. But I also updated it to version 5.10 (it was written in v3.9.3). Should I open another PR for this?

Yes please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants