DOC: random: fix doc linking, was referencing private submodules. #14360
Conversation
| @@ -190,7 +190,7 @@ Concepts | |||
| :maxdepth: 1 | |||
|
|
|||
| generator | |||
| legacy mtrand <legacy> | |||
| Legacy generator and functions <legacy> | |||
There was a problem hiding this comment.
To me this is confusing, mtrand is its own class and does not inherit from generator
There was a problem hiding this comment.
Maybe more verbose is better: "Legacy generator (mtrand) and functions" ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like Legacy Generator or Legacy Generator (RandomState) which might help some users familiar with RandomState. Not sure and functions feels meaningful.
There was a problem hiding this comment.
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.
|
The rendered legacy page looks good, I did not see any broken links in italics. |
|
Also Cc @bashtage. Thoughts on the namespace / public API questions in the PR description? |
| .. module:: numpy.random.mt19937 | ||
|
|
||
| .. currentmodule:: numpy.random.mt19937 | ||
| .. currentmodule:: numpy.random |
There was a problem hiding this comment.
Are you sure current module needs to change? Doc render looks right. The module isn't private (numpy.random.mt19937).
There was a problem hiding this comment.
Same comment for the other bit generators.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| @@ -190,7 +190,7 @@ Concepts | |||
| :maxdepth: 1 | |||
|
|
|||
| generator | |||
| legacy mtrand <legacy> | |||
| Legacy generator and functions <legacy> | |||
There was a problem hiding this comment.
I like Legacy Generator or Legacy Generator (RandomState) which might help some users familiar with RandomState. Not sure and functions feels meaningful.
|
@rgommers this still has a TODO in the first comment |
Addressed. More importantly, there's still no feedback from anyone on the "to decide" points in the PR description. |
|
Perhaps we should merge this as-is and discuss what should be exposed in the namespace in issue #14359 or in a new issue |
Okay, let's do that. Thanks Matti |
Addresses part of gh-14359
TODO / to decide: there's still two namespaces left that look private:
random_entropyandseed_by_array, unclear if those need to be public)BitGeneratoras a public object I think)