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

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

Merged
merged 2 commits into from
Sep 7, 2019

Conversation

rgommers
Copy link
Member

@rgommers rgommers commented Aug 25, 2019

Addresses part of gh-14359

TODO / to decide: there's still two namespaces left that look private:

  • numpy.random.entropy (contains random_entropy and seed_by_array, unclear if those need to be public)
  • numpy.random.bit_generator (contains only BitGenerator as a public object I think)

@@ -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.

@mattip
Copy link
Member

mattip commented Aug 25, 2019

The rendered legacy page looks good, I did not see any broken links in italics.

@rgommers rgommers changed the title DOC: fix doc linking, was referencing private submodules. DOC: random: fix doc linking, was referencing private submodules. Aug 25, 2019
@rgommers
Copy link
Member Author

Also Cc @bashtage. Thoughts on the namespace / public API questions in the PR description?

doc/source/reference/random/legacy.rst Show resolved Hide resolved
.. 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.

@@ -190,7 +190,7 @@ Concepts
:maxdepth: 1

generator
legacy mtrand <legacy>
Legacy generator and functions <legacy>
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.

@mattip
Copy link
Member

mattip commented Aug 31, 2019

@rgommers this still has a TODO in the first comment

@rgommers
Copy link
Member Author

rgommers commented Sep 1, 2019

@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.

@mattip
Copy link
Member

mattip commented Sep 1, 2019

Perhaps we should merge this as-is and discuss what should be exposed in the namespace in issue #14359 or in a new issue

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Sep 5, 2019
@charris charris added this to the 1.17.3 release milestone Sep 5, 2019
@rgommers
Copy link
Member Author

rgommers commented Sep 7, 2019

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

@rgommers rgommers merged commit 711440c into numpy:master Sep 7, 2019
@rgommers rgommers deleted the random-docs branch September 7, 2019 16:56
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Sep 14, 2019
@charris charris removed this from the 1.17.3 release milestone Sep 14, 2019
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.

None yet

4 participants