8000 DOC: random: fix doc linking, was referencing private submodules. by rgommers · Pull Request #14360 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

DOC: random: fix doc linking, was referencing private submodules. #14360

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 7, 2019
Merged

DOC: random: fix doc linking, was referencing private submodules. #14360

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d882d39
DOC: fix doc linking, was referencing private submodules.
rgommers Aug 25, 2019
e4324a7
DOC: address last comment on numpy.random doc page fixes PR
rgommers Sep 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
DOC: fix doc linking, was referencing private submodules. 
Closes gh-14359
  • Loading branch information
@rgommers
rgommers committed Aug 25, 2019
commit d882d3931ff5e88c528ee72f120786ee6a52e47c
6 changes: 2 additions & 4 deletions doc/source/reference/random/bit_generators/mt19937.rst
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
Mersenne Twister (MT19937)
Mersenne Twister (MT19937)
--------------------------

.. module:: numpy.random.mt19937

.. currentmodule:: numpy.random.mt19937
.. currentmodule:: numpy.random
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure current module needs to change? Doc render looks right. The module isn't private (numpy.random.mt19937).

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for the other bit generators.

Copy link
Member Author

Choose a reason for hiding this comment

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

The module isn't private

unfortunately we have never used underscores correctly (or much at all), which I think is what you're referring to as "private". but that's definitely a private submodule.

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't my intent to make it private. There are use cases where someone wants to consuma a bit generator but doesn't want anything else (.e.g writing their own low-level Generator-like objects). Should they be directed to numpy.random.init. Also, there are a lot of intentionally private parts to random, so I think there was some consideration about what should and shouldn't be private here (or what should be stable and what is not).

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a couple of issues with that:

  • We shouldn't have one object in two public places (np.random.MT19937 is np.random.mt19937.MT19937)
  • Deep nesting isn't great (see import this - flat is better than nested)
  • Public submodules with a single object in them don't name too much sense as API design.

Also, this was never discussed I believe. You can't just add many new submodules to the numpy API without any proposal or discussion. My understanding was that all public functions and classes were added to the numpy.random namespace. Hence my question in the summary of this PR about the few functions that are missing there - were they intended to be public or not?

Should they be directed to numpy.random.__init__.

I don't see a problem with that. np.random.MT19937 is clearer than np.random.mt19937.MT19937.


.. autoclass:: MT19937
:exclude-members:
Expand Down
4 changes: 1 addition & 3 deletions doc/source/reference/random/bit_generators/pcg64.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
Parallel Congruent Generator (64-bit, PCG64)
--------------------------------------------

.. module:: numpy.random.pcg64

.. currentmodule:: numpy.random.pcg64
.. currentmodule:: numpy.random

.. autoclass:: PCG64
:exclude-members:
Expand Down
4 changes: 1 addition & 3 deletions doc/source/reference/random/bit_generators/philox.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
Philox Counter-based RNG
------------------------

.. module:: numpy.random.philox

.. currentmodule:: numpy.random.philox
.. currentmodule:: numpy.random

.. autoclass:: Philox
:exclude-members:
Expand Down
4 changes: 1 addition & 3 deletions doc/source/reference/random/bit_generators/sfc64.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
SFC64 Small Fast Chaotic PRNG
-----------------------------

.. module:: numpy.random.sfc64

.. currentmodule:: numpy.random.sfc64
.. currentmodule:: numpy.random

.. autoclass:: SFC64
:exclude-members:
Expand Down
2 changes: 1 addition & 1 deletion doc/source/reference/random/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ Concepts
:maxdepth: 1

generator
legacy mtrand <legacy>
Legacy generator and functions <legacy>
Copy link
Member

Choose a reason for hiding this comment

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

To me this is confusing, mtrand is its own class and does not inherit from generator

Copy link
Member

Choose a reason for hiding this comment

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

Maybe more verbose is better: "Legacy generator (mtrand) and functions" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

mtrand is a private implementation detail, it should not be mentioned anywhere. Happy to change it if you have another suggestion, but I think this is about as accurate and clear as it gets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like Legacy Generator or Legacy Generator (RandomState) which might help some users familiar with RandomState. Not sure and functions feels meaningful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I missed the actual point here: I thought that page had numpy.random.<function> links, but it doesn't. That shows a bigger problem though, those functions (e.g. np.random.seed, np.random.randn) are completely missing from the docs.

that's a larger fix, so for now I'll just push a commit to address this comment, then this PR can be merged.

BitGenerators, SeedSequences <bit_generators/index>

Features
Expand Down
18 changes: 8 additions & 10 deletions doc/source/reference/random/legacy.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,27 @@

Legacy Random Generation
------------------------
The `~mtrand.RandomState` provides access to
The `RandomState` provides access to
legacy generators. This generator is considered frozen and will have
no further improvements. It is guaranteed to produce the same values
as the final point release of NumPy v1.16. These all depend on Box-Muller
normals or inverse CDF exponentials or gammas. This class should only be used
if it is essential to have randoms that are identical to what
would have been produced by previous versions of NumPy.

`~mtrand.RandomState` adds additional information
`RandomState` adds additional information
to the state which is required when using Box-Muller normals since these
are produced in pairs. It is important to use
`~mtrand.RandomState.get_state`, and not the underlying bit generators
`RandomState.get_state`, and not the underlying bit generators
`state`, when accessing the state so that these extra values are saved.

Although we provide the `~mt19937.MT19937` BitGenerator for use independent of
`~mtrand.RandomState`, note that its default seeding uses `~SeedSequence`
rather than the legacy seeding algorithm. `~mtrand.RandomState` will use the
Although we provide the `MT19937` BitGenerator for use independent of
`RandomState`, note that its default seeding uses `SeedSequence`
rather than the legacy seeding algorithm. `RandomState` will use the
legacy seeding algorithm. The methods to use the legacy seeding algorithm are
currently private as the main reason to use them is just to implement
`~mtrand.RandomState`. However, one can reset the state of `~mt19937.MT19937`
using the state of the `~mtrand.RandomState`:
`RandomState`. However, one can reset the state of `MT19937`
using the state of the `RandomState`:

.. code-block:: python

Expand All @@ -47,8 +47,6 @@ using the state of the `~mtrand.RandomState`:
rs2.standard_exponential()


.. currentmodule:: numpy.random.mtrand

.. autoclass:: RandomState
:exclude-members:

Expand Down
4 changes: 2 additions & 2 deletions numpy/random/mtrand.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ cdef class RandomState:
See Also
--------
Generator
mt19937.MT19937
Bit_Generators
MT19937
:ref:`bit_generator`

"""
cdef public object _bit_generator
Expand Down
0