Skip to content

DOC: Revise glossary page#16996

Merged
mattip merged 24 commits intonumpy:masterfrom
bjnath:revised-glossary
Sep 28, 2020
Merged

DOC: Revise glossary page#16996
mattip merged 24 commits intonumpy:masterfrom
bjnath:revised-glossary

Conversation

@bjnath
Copy link
Contributor

@bjnath bjnath commented Aug 3, 2020

Expanded and updated the Glossary page in an effort to make it more useful.

This is a starting place; I welcome the community's comments regarding clarity, choice of entries, length of definitions, technical accuracy, etc.

bjnath added a commit to bjnath/numpy-1 that referenced this pull request Aug 3, 2020
More clearly describe `shares_memory` as being
not always *feasible* rather than *possible*
bjnath added a commit to bjnath/numpy-1 that referenced this pull request Aug 4, 2020
Also reworded `stride` entry for clarity, moved
'axis' entry to correct alphabetical position.

Also added anchors to reference/arrays.indexing for glossary references
to point to.
@bjnath
Copy link
Contributor Author

bjnath commented Aug 4, 2020

Thank you, @BvB93, excellent suggestions. One goal of the rewrite was to give more visibility to the reference pages, so this is very welcome.

All suggestions incorporated as-is except for the last (#16996 (comment)) -- if the user clicks the copy link, there's only "See views."

Also clarified stride entry, fixed alphabetization of axis, and replaced two URLs with :ref: links.

@BvB93
Copy link
Member

BvB93 commented Aug 4, 2020

All suggestions incorporated as-is except for the last (#16996 (comment)) -- if the user clicks the copy link, there's only "See views."

Huh, odd. Does the same issue apply to the :term:`view` references used earlier on in the document?

@BvB93
Copy link
Member

BvB93 commented Aug 4, 2020

Btw, there appear to be a couple of doctest-related issues (link).
I suspect that these errors are preventing circle from rendering the updated docs.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 5, 2020

@BvB93

if the user clicks the copy link, there's only "See views."

Sorry, I wasn't clear. The glossary handles both copy and view in the view entry and the copy entry just says 'see view'.

As a result, there's no value in giving copy a :term: role inside the view entry, since the link points to the copy entry which just says to look at the view entry.

bjnath added a commit to bjnath/numpy-1 that referenced this pull request Aug 5, 2020
@bjnath
Copy link
Contributor Author

bjnath commented Aug 5, 2020

@WarrenWeckesser
Copy link
Member

Nice!

bjnath added a commit to bjnath/numpy-1 that referenced this pull request Aug 5, 2020
strctured -> structured, caught by @WarrenWeckesser.

Fixed missing language in `view` entry.
@BvB93
Copy link
Member

BvB93 commented Aug 5, 2020

References to the old doc.glossary module will also have to be removed from certain tests (ref).

Not entirely sure though if we're allowed to straight up remove the old doc.glossary module; it is apparently part of the public API.
A second opinion would be very much appreciated here.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 5, 2020

@WarrenWeckesser -- Thank you!

@seberg
Copy link
Member

seberg commented Aug 5, 2020

I think we may be able to pull removal of, maybe discuss it today. But for example debian strips the whole doc module to begin with and nobody seems to have ever complained. And I guess that np.info (which can pick these up) is rarely to never used.

bjnath added a commit to bjnath/numpy-1 that referenced this pull request Aug 5, 2020
Also adding <BLANKLINE>s to some examples for clarity.
array_like

Any :doc:`scalar <reference/arrays.scalars>` or
`sequence <https://docs.python.org/3/glossary.html#term-sequence>`_
Copy link
Member

Choose a reason for hiding this comment

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

Can be done in a follow-up, but would be nice to replace these with intersphinx links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered if there weren't a better way to handle these. I'm glad to fix it right away. What should it say?

Copy link
Member

Choose a reason for hiding this comment

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

My guess would be :term:`python:sequence` , but that's only a guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a working link, because we have an intersphinx link in conf.py:

      :ref:`slice <python:glossary>`

but we want to link to the slice entry, not just the page. This gives "undefined label":

      :ref:`slice <python:glossary#term-slice>`

as does this:

      :ref:`slice <python:glossary:term:slice>`

When it comes to half-assed, Sphinx is without peer.

Leaving the URLs for now.

Copy link
Member

Choose a reason for hiding this comment

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

Did my approach fail?

This seems to suggest it should work: https://stackoverflow.com/q/46473481/102441

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and swapped all the explicit links using intersphinx in c24bd and 690037.

I'm against duplicating the terms from the Python glossary in NumPy's glossary, as it just leads to more clicks for the user. Nevertheless I switched them to intersphinx links in 6900372 and leave the decision whether to include them to others.

@bjnath a helpful resource for working with intersphinx is to inspect the objects.inv file, which describes the mapping from the sphinx names to URLs. It's not the most visible thing in the world, but the intersphinx docs explain how to grab objects.inv for inspection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did my approach fail?

The approach works as expected! I went ahead and applied it throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I stole the half-assed prize. Sorry @eric-wieser, had the answer right in front of me. Thanks @rossbar for the fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also @rossbar thanks for the objects.inv tip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll write a guide to linking for NumPy docs writers, including stuff like neps and of course this one.

bjnath added a commit to bjnath/numpy-1 that referenced this pull request Aug 5, 2020
@bjnath
Copy link
Contributor Author

bjnath commented Aug 5, 2020

@rossbar:

I'm against duplicating the terms from the Python glossary in NumPy's glossary, as it just leads to more clicks for the user

Linking to the Python glossary alerts you that the unfamiliar term isn't NumPy, it's Python, and it takes you directly to the definition.

It's easy to imagine technical users who are new to both Python and NumPy. It's a good deed and good documentation to help them sort it out. We can do it adequately with a small number of links.

Neither of these sounds better:

  • Copy the Python terms into our glossary

  • Leave the terms out and provide no clue where to look

Also, could you help me understand this change?

DOC: rm tuple from glossary

I kept many of the Python terms from the original glossary and perhaps we don't need them all, though it's not clear what harm they do.

Why the beef with tuple? It's pervasive in NumPy and seems worth setting straight for anyone who thinks they know what it is but isn't sure.

@rossbar
Copy link
Contributor

rossbar commented Aug 5, 2020

Neither of these sounds better:

Agreed - I would prefer that we use intersphinx for the terms where they appear. In other words, if in the documentation it says e.g. ndarray has the attribute shape, instead of linking to our glossary via ndarray has the :term:`attribute` shape, we do ndarray has the :term:`python:attribute` shape. The reason being that the former takes you to a link in the numpy glossary, which is itself a link to the Python glossary, requiring multiple clicks to get to the definition. I think it would be better if it were a single click. This should be an easy find/replace for the duplicate terms.

Also, could you help me understand this change?

It turns out there is no glossary entry for "tuple" in the Python glossary!

@bjnath
Copy link
Contributor Author

bjnath commented Aug 5, 2020

Agreed - I would prefer that we use intersphinx for the terms where they appear. In other words, if in the documentation it says e.g. ndarray has the attribute shape, instead of linking to our glossary via ndarray has the :term:attribute shape, we do ndarray has the :term:python:attribute shape

Thanks for explaining this. I'd intended for them to work this way. I will fix it. But we can still keep the Python links to help people who encounter those terms in other NumPy docs.

no glossary entry for "tuple"

Two "d'ohs" in a single PR! Thanks for catching this. I did try to check the links but need to do it more effectively.

@bjnath
Copy link
Contributor Author

bjnath commented Aug 5, 2020

@rossbar -- So to be clear, your wish is that when a non-glossary file has a Python :term:, it should link directly to the Python glossary rather than to ours? That would be my preference too.

A quick grep for :term: doesn't seem to turn up many, or possibly any.

Actually most of the :term: links seem silly; they're from files that themselves define the terms; visiting the glossary won't turn up anything that hasn't already been explained in the file. I'd like to fix these, but in another PR.

Since there don't seem to be many long-way-around Python links, and the existing Python links might serve a useful purpose, are you OK on leaving the Python links in the glossary?

@rossbar
Copy link
Contributor

rossbar commented Aug 6, 2020

So to be clear, your wish is that when a non-glossary file has a Python :term:, it should link directly to the Python glossary rather than to ours?

Yes, linking directly to the relevant glossary would be preferable.

Since there don't seem to be many long-way-around Python links, and the existing Python links might serve a useful purpose, are you OK on leaving the Python links in the glossary?

I don't feel strongly about it - my main concern was with the multiple-redirection problem. If we feel it provides useful context to have stubs in the NumPy glossary to point to the Python glossary then that's fine by me!

@mattip
Copy link
Member

mattip commented Aug 12, 2020

I think we are ok with this change which will clear the test failures

--- a/numpy/tests/test_public_api.py
+++ b/numpy/tests/test_public_api.py
@@ -151,7 +151,6 @@ PUBLIC_MODULES = ['numpy.' + s for s in [
     "doc.constants",
     "doc.creation",
     "doc.dispatch",
-    "doc.glossary",
     "doc.indexing",
     "doc.internals",
     "doc.misc",

@mattip
Copy link
Member

mattip commented Aug 19, 2020

CI is passing. I think this is ready to merge.

type
In NumPy, a synonym for :term:`dtype`. For the more general Python
meaning, :term:`see here. <python:type>`

Copy link
Member

Choose a reason for hiding this comment

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

This entry has dissapeared on master, so I removed it from the PR as well. OK?

Copy link
Contributor Author

@bjnath bjnath Sep 16, 2020

Choose a reason for hiding this comment

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

This was new content, not meant to be removed. I see I'm author again, so will fix.

@mattip
Copy link
Member

mattip commented Sep 16, 2020

Merged the master HEAD into this branch and resolved the differences.

Comment on lines 99 to 100
The result of an operation along an :term:`axis` X is an array in which X
disappears. This can surprise new users expecting the opposite.
Copy link
Member

@eric-wieser eric-wieser Sep 16, 2020

Choose a reason for hiding this comment

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

This description is inaccurate:

>>> x = np.arange(12).reshape(3, 4)
>>> x.shape
(3, 4)
>>> np.sort(x, axis=0).shape  # sort along axis 0
(3, 4)  # axis didn't disappear!

It would be accurate as

Suggested change
The result of an operation along an :term:`axis` X is an array in which X
disappears. This can surprise new users expecting the opposite.
The result of an reduction along an :term:`axis` X is an array in which X
disappears. This can surprise new users expecting the opposite.

but I suspect you want this glossary entry to describe the more general case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Alternative in 511d769 .

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Seems like the merge didn't go cleanly in a few places.

:doc:`numpy.array <reference/generated/numpy.array>`
is array_like. ::

>>> a = np.array([[1, 2.0],[0, 0],(1+1j, 3.)])
Copy link
Member

Choose a reason for hiding this comment

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

PEP8

Suggested change
>>> a = np.array([[1, 2.0],[0, 0],(1+1j, 3.)])
>>> a = np.array([[1, 2.0], [0, 0], (1+1j, 3.)])

>>> x + y
array([[4, 5],
[5, 6]])
Ordinarily this means the shapes of a and b must be identical. But in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ordinarily this means the shapes of a and b must be identical. But in
Ordinarily, this means the shapes of a and b must be identical. But in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sentence adverbs usually need the comma:

Frankly, that's impossible.

but in this case it's stylistic:

Ordinarily X is rational; on weekends it's irrational.
Ordinarily, X is rational; on weekends, it's irrational.

and we can imagine a single text having both:

We show that usually X is rational but on weekends it's irrational.
Usually, X is rational, but, on weekends, it is irrational. QED.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for giving me the name of this construction.

Both https://www.grammarly.com/blog/commas-after-introductory-words/ and https://theeditorsblog.net/2016/02/21/a-tale-of-adverbs-and-the-comma/ suggest that the comma should be present, so I'd err towards matching the norm rather than flouting stylistic freedom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't define "norms" based on what's on the internet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The guidance given by the Grammarly program (pg88) appears to be:

Introductory clauses and long phrases must always be followed by a comma, while single words and short phrases may or may not.

The second reference doesn't seem to say anything about this case.

Neither one seems to be supporting the comma campaign.

Also added "contiguous," which is referenced in docs but hadn't been
defined.
@bjnath
Copy link
Contributor Author

bjnath commented Sep 17, 2020

Pushed update to address reviewer comments above. Also adds "contiguous," which had not been in the glossary but had a reference in the docs.

https://15810-908607-gh.circle-artifacts.com/0/doc/build/html/glossary.html

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Conflicts look resolved now, thanks.

@@ -37,8 +41,8 @@ Glossary

It can be used at most once; ``a[...,0,...]`` raises an :exc:`IndexError`.
Copy link
Member

Choose a reason for hiding this comment

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

This line needs indenting to fall inside the lost item

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

Thanks -- fixed in 90111e2 .

[ 7, 6, 5, 4],
[11, 10, 9, 8]])

An operation "along axis 0" behaves as if the argument were an array of
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice description. I think you should start with this instead of the reversing example

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

Good suggestion; rewritten in 90111e2 .


[1, 4, 2, 5, 3, 6]
contiguous
An array is contiguous if it occupies a single unbroken block of memory.
Copy link
Member

Choose a reason for hiding this comment

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

Typically the requirement is stronger than this - strides must also be non-negative, with later elements appearing after earlier ones.

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

Thanks -- modified in 90111e2

A :term:`structured data type` may contain a :term:`ndarray` with its
own dtype and shape:
subarray
An array nested in a :term:`structured data type`: ::
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An array nested in a :term:`structured data type`: ::
An array nested in a :term:`structured data type`, as `b` is here::

The colon is added automatically by ::, no need to repeat jt

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

Change made in 90111e2 .

any number of other arrays with different types, shapes, and even
content. This is much faster than creating those arrays.

An array created this way is a `view`, and the performance gain often
Copy link
Member

Choose a reason for hiding this comment

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

I think this will link to ndarray.view, which isn't really the right meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just shows as italic.

every second element of another array::
Without changing underlying data, NumPy can make one array masquerade as
any number of other arrays with different types, shapes, and even
content. This is much faster than creating those arrays.
Copy link
Member

Choose a reason for hiding this comment

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

"content" (at least, in terms of bitpattern) is the the one thing that cant change.

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

I'm not sure what I was trying to convey. You're right. Changed in 90111e2 .

Used as a dimension entry, ``-1`` instructs NumPy to choose the length
that will keep the total number of elements the same.

>>> np.arange(12).reshape(4,-1).shape
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> np.arange(12).reshape(4,-1).shape
>>> np.arange(12).reshape(4, -1).shape

Perhaps also worth linking to the python behavior where a[-1] = a[len(a)-1], and mentioning that applies to axes not just data.

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

Added in 90111e2 .

I had great fun trying to get intersphinx to work on the python link. conf.py says

intersphinx_mapping = {
    'python': ('https://docs.python.org/dev', None),

and calling

 python -m sphinx.ext.intersphinx https://docs.python.org/dev/objects.inv

shows

std:doc
        faq/programming                          Programming FAQ                         : faq/programming.html

so you'd think

doc:`python:Programming FAQ`

would work. It didn't. Even if it did, it wouldn't link to the relevant entry.

Maybe I missed something. Happy to replace the URL with something Sphinx that works.

Copy link
Member

Choose a reason for hiding this comment

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

doc:`python:faq/programming` ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. It doesn't seem to work with the anchor, though, so we're torn between a politically correct link that leaves you at the top of the page or a politically incorrect link that takes you to what we want you to see.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing the Sphinx link you're referring to in this diff context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I chose to keep the link that went directly to the entry, even though it was a URL.

Copy link
Member

Choose a reason for hiding this comment

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

Using the url looks like the right choice here, unless Sphinx has some hybrid "append this path to the base url of this intersphinx target". Avoiding urls is most important for internal links. For external ones like these, the only danger is linking to an outdated python doc version.

array([[ 0, 1, 2, 3],
[ 4, 5, 6, 7],
[ 8, 9, 10, 11]])
<BLANKLINE>
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a misuse of blankline - numpy does not emit a blank line here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be misleading if the text were part of a docstring, but here it's used for formatting the documentation and is invisible to the reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, missed your point initially. Agreed, will change.

Copy link
Contributor Author

@bjnath bjnath Sep 18, 2020

Choose a reason for hiding this comment

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

Removed in 90111e2 .

@bjnath
Copy link
Contributor Author

bjnath commented Sep 18, 2020

@bjnath
Copy link
Contributor Author

bjnath commented Sep 26, 2020

Are we ready to take the final step and merge this?

large arrays. To see the entire array, use `numpy.printoptions`


``:``
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for dropping the code formatting?

@bjnath
Copy link
Contributor Author

bjnath commented Sep 26, 2020

@eric-wieser

Any reason for dropping the code formatting?

I'd added the styling for visibility, since the punctuation headings are hard to see. I dropped it since the new theme shrinks the code font, making the headings even harder to see.

@eric-wieser
Copy link
Member

I'd added the styling for visibility, since the punctuation headings are hard to see.

Perhaps ": (symbol)"?

@bjnath
Copy link
Contributor Author

bjnath commented Sep 26, 2020

I'd added the styling for visibility, since the punctuation headings are hard to see.

Perhaps ": (symbol)"?

Why not, gave it a shot.

image

"(symbol)" overachieves the goal of catching the eye and swallows the symbol. There's no patching up the fact that the monospaced font is too small.

I can't think of a compelling rationale for using one format or the other, so I'd like the freedom to pick whatever's easiest to see.

@eric-wieser
Copy link
Member

Thanks for trying that out, I agree it looks better as is.


**When indexing an array**, shorthand that the missing axes, if they
exist, are full slices.
. . .
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for the spaces here? That's likely to frustrate users searching for ... on this page.

Suggested change
. . .
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legibility, of course. I thought it more likely that someone would eyeball the glossary for the ellipsis, especially since it's on top of the page, than make the effort of searching with the browser.

Sphinx search doesn't find either version.

Here's how it looks without spaces:

image

I think the spaces help more users than they hurt.

@bjnath
Copy link
Contributor Author

bjnath commented Sep 27, 2020

Do any issues remain, or can this be merged?

@mattip
Copy link
Member

mattip commented Sep 28, 2020

Let's merge this as-is, and tweak it in subsequent PRs

@mattip mattip merged commit 6525ed5 into numpy:master Sep 28, 2020
@mattip
Copy link
Member

mattip commented Sep 28, 2020

Thanks @bjnath

@bjnath
Copy link
Contributor Author

bjnath commented Sep 28, 2020

And thank you, @mattip, for untangling and re-threading what turned out to be a much more complicated PR than I thought. If you hadn't stepped in I'd still be looking at the pieces like a dazed Ikea purchaser. I appreciate your putting thought and effort into making this merge possible.

@bjnath
Copy link
Contributor Author

bjnath commented Sep 29, 2020

And thanks also to reviewers @BvB93 , @WarrenWeckesser, @rossbar, and @eric-wieser!

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.

7 participants