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

Ensure reference renaming is parallel-safe #136

Merged
merged 11 commits into from
Mar 28, 2018

Conversation

jnothman
Copy link
Member

@jnothman jnothman commented Nov 1, 2017

Fixes #112. Builds upon #135. Makes #130 (breaking table whitespace) worse, but see below.

Also fixes #114

At autodoc-process-docstring time, this prefixes each reference with a token indicative of which docstring it belongs to and then relabels the text of the reference in the doctree once the reST is parsed. The rendered reference naming therefore reflects what was in the original reST, rather than something with prefixes etc. So you get [1] rather than [R1].

I'm not sure how to rigorously test this but will post some images soon.

The issue of breaking table whitespace (#130) is more minor than #112. It is exacerbated by this patch since references become much longer. There may be a more ideal solution that involves post-processing the doctrees to do all of the reference renaming (i.e. without this decorate-and-undecorate approach), but when I tried to implement it, I realised it would involve hacking footnotes to become citations, handling initial broken resolutions (incl docutils.nodes.problematic), and other madness. It may come later. In the meantime I think we should just tell users that citations cannot be used in tables.

@jnothman jnothman force-pushed the rename-refs-decorate-undecorate branch from c066e9c to 53efcb0 Compare November 1, 2017 09:50
@jnothman
Copy link
Member Author

jnothman commented Nov 1, 2017

At scikit-learn stable (old numpydoc):

screen shot 2017-11-01 at 9 01 09 pm

At scikit-learn dev (numpydoc 0.7):

screen shot 2017-11-01 at 9 01 49 pm

At this branch:

screen shot 2017-11-01 at 9 02 54 pm

The reference definitions correspond in text, and the hyperlinks work.

@astrofrog
Copy link

This is a neat solution!

This prefixes each reference with a token indicative of which docstring it belongs to and then relabels the text of the reference once the doctree is compiled
@jnothman jnothman force-pushed the rename-refs-decorate-undecorate branch from 9f57ecb to 81986b0 Compare November 1, 2017 11:21
* Use label text not normalised text
* Split at first - not last
* Do not relabel non-docstring content
@jnothman
Copy link
Member Author

jnothman commented Nov 1, 2017

Again, would be nice to have a more robust way to test, but I've ironed out a few issues I noticed... This is ready for review.

@jnothman jnothman mentioned this pull request Nov 1, 2017
@stefanv
Copy link
Contributor

stefanv commented Nov 1, 2017

Any reason not to use hexadecimal or base 36? The above would become 5AAAE and 7YJY respectively.

@jnothman
Copy link
Member Author

jnothman commented Nov 1, 2017

I'm not sure what you mean here by "the above". The digest currently used is in hexadecimal. But I suppose you mean to include the whole name of the object encoded in base36?

@jnothman
Copy link
Member Author

jnothman commented Nov 1, 2017

I'm not sure how you propose to encode a string in base36. That doesn't seem to be a standard thing to do. Or do you mean to encode a hash in base36?? Why better than hex?

@stefanv
Copy link
Contributor

stefanv commented Nov 1, 2017

@jnothman Sorry for the noise, I misread your screenshot above as an example of how things are. Your results look perfect.

@jnothman
Copy link
Member Author

jnothman commented Nov 1, 2017

I'd like to check a tex build before merge...

@jnothman
Copy link
Member Author

jnothman commented Nov 2, 2017

It almost works in TeX... because the references are (by default) in end-notes, the entries with the same key are indistinguishable by key... which looks a bit weird, but at least the links work. Would be better if references from docstring weren't moved to a single bibliography...

@jnothman
Copy link
Member Author

jnothman commented Nov 8, 2017

We can leave this (or an alternative) fix until after releasing 0.8, and set parallel_read_safe=False for now. Or are we happy with this despite it making LaTeX bibliographies less useful?

@jnothman
Copy link
Member Author

Any opinion on whether we release 0.8 with this (despite any degradation to LaTeX bibliographies) or with parallel_read_safe=False?

@rgommers
Copy link
Member

Just based on the description in the PR (didn't test or review in detail): I'd be okay with merging this.

@rgommers
Copy link
Member

I set myself a reminder to check this on the numpy and scipy html/latex builds in the second week of January. If that all looks good then I'm going to merge this, unless there's other opinions by then. (of course if someone wants to merge it sooner after reviewing, please do)

@astrofrog
Copy link

@rgommers - just to check, have you had any further thoughts on this?

@jnothman
Copy link
Member Author

A note that this is being discussed at sphinx-doc/sphinx#4782 (comment). @jfbu seems to think that this PR is looking like a reasonable solution for now.

@rgommers rgommers added this to the v0.8.0 milestone Mar 28, 2018
@rgommers
Copy link
Member

Two months late: no further thoughts from me. Still no 0.8.0, I forgot about it during my relocation, Should do that soon.

@stefanv
Copy link
Contributor

stefanv commented Mar 28, 2018 via email

@jnothman
Copy link
Member Author

Let's take this as consensus to merge, then

@jnothman jnothman merged commit fb6afac into numpy:master Mar 28, 2018
@prisae prisae mentioned this pull request May 4, 2018
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.

Unnecessary renaming of references BUG: Reference de-duplication isn't parallel-safe
4 participants