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.

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

Jump to bottom

ENH: Add typing for RandomState #18433

Merged
merged 14 commits into from
Feb 24, 2021
Merged

ENH: Add typing for RandomState #18433

merged 14 commits into from
Feb 24, 2021

Conversation

bashtage
Copy link
Contributor

Add typing for RandomState
Small fixes to Generator's typing

@BvB93 BvB93 self-requested a review February 17, 2021 15:40
@BvB93
Copy link
Member

BvB93 commented Feb 17, 2021

Closes #17103.

@BvB93
Copy link
Member

BvB93 commented Feb 17, 2021

I think that it would be wise to add a few typing tests for np.random at this point.

There are currently three types of typing tests: reveal, pass and fail:

  • The reveal tests check whether the observed mypy reveal is indeed what one would expect (example).
  • The pass tests check whether the code passes without any errors, be it errors produced by mypy or by actually executing the code during runtime (example).
  • The fail tests check that faulty expressions return the mypy error that one might expect of them (example).

@bashtage bashtage closed this Feb 18, 2021
@bashtage bashtage reopened this Feb 18, 2021
@bashtage bashtage force-pushed the randomstate-typing branch 2 times, most recently from df17793 to 36bd237 Compare February 19, 2021 11:59
@BvB93
Copy link
Member

BvB93 commented Feb 19, 2021

At first glance this PR looks just fine; I'll give it a final once-over later this week.

@bashtage bashtage force-pushed the randomstate-typing branch 5 times, most recently from 6bb887b to 70e7bd5 Compare February 20, 2021 22:27
@bashtage
Copy link
Contributor Author

bashtage commented Feb 20, 2021 •
edited

@BvB93 I'm struggling with functions that have outputs ndarray[Any, type[int_]] since int_ has different sizes on different platforms. Can these be typed checked using reveal? Or do I need to write different tests for Windows and Linux? Or something else?

These are all Windows or Linux 32 tests.

@bashtage
Copy link
Contributor Author

@BvB93 I found your typing template in the tests, e.g. {int_}.

BvB93
BvB93 reviewed Feb 23, 2021
View reviewed changes
Copy link
Member

@BvB93 BvB93 left a comment

Choose a reason for hiding this comment

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

@bashtage it took a bit longer than expected but I've made some time to go through the PR.
There are a few comments but overall I'd say it's looking pretty good.

numpy/random/_generator.pyi Show resolved Hide resolved
numpy/typing/tests/data/fail/random.py Show resolved Hide resolved
numpy/random/mtrand.pyi Outdated Show resolved Hide resolved
numpy/random/mtrand.pyi Outdated Show resolved Hide resolved
numpy/random/mtrand.pyi Outdated Show resolved Hide resolved
numpy/random/mtrand.pyi Outdated Show resolved Hide resolved
@BvB93
Copy link
Member

BvB93 commented Feb 23, 2021

@BvB93 I found your typing template in the tests, e.g. {int_}.

If you ever stumble upon a common pattern that you feel might be useful to have as an alias,
then go ahead add it to the dictionary.

@bashtage bashtage force-pushed the randomstate-typing branch 2 times, most recently from 2b00243 to 1e3d5e0 Compare February 23, 2021 22:09
BvB93
BvB93 reviewed Feb 24, 2021
View reviewed changes
numpy/random/mtrand.pyi Outdated Show resolved Hide resolved
@bashtage
ENH: Add typing for RandomState
c2207b9 
Add typing for RandomState
Small fixes to Generator's typing
@bashtage
MAINT: Restructure imports in pyi file
c21889f
@bashtage
TST: Add tests for random typing
99c7c04
@bashtage
ENH/BUG: Add tests and correct typing issues
95d1f23 
Add tests for lower level components
Correct errors found in testing
Improve specificity of tests
Other small fixes to docs and typing
bashtage and others added 10 commits February 24, 2021 10:40
@bashtage
ENH/BUG: Improve definitions and fix bugs
69de25e 
Add tests
Fix related bugs
Improve specificity of definitions
@bashtage
TST: Add tests for dtype and out
d7bc29c 
Add further tests for functions that accept out and dtype arguments
@bashtage
BUG: Corect typing statements in mtrand
7f2db30 
Bring over changes from Generator to RandomState where appropriate
Remove attributes not in RandomState
@bashtage
ENH: Add tests for RandomState
2e92a52
@bashtage
BUG: Fix typing bugs
a05d5db
@bashtage
Small fixes
3b561dd
@bashtage
Use {int_} for int return types
4e3d7ed
@bashtage
TST: Add final tests
c282c31
@bashtage
CLN: Clean up random typing
dc8481b 
Clean unnecessary definitions
Fix small typing errors
@bashtage
BUG: Remove incorrect variable name
d4c1236 
Remove incorrect variable name in randint
BvB93
BvB93 approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@BvB93 BvB93 left a comment

Choose a reason for hiding this comment

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

LGTM

mattip
mattip reviewed Feb 24, 2021
View reviewed changes
numpy/random/__init__.pyi
@@ -8,57 +8,58 @@ from numpy.random._philox import Philox as Philox
from numpy.random._sfc64 import SFC64 as SFC64
from numpy.random.bit_generator import BitGenerator as BitGenerator
from numpy.random.bit_generator import SeedSequence as SeedSequence
from numpy.random.mtrand import (
Copy link
Member

Choose a reason for hiding this comment

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

Does importing here work differently than regular python that we need to do "from x import y as y" and not "from x import y"? If so, it would be nice to get a comment explaining this for the next time I ask the same question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking @BvB93 's word, yes. If you want a symbol reexported, then you need to import X as X.

Copy link
Member

Choose a reason for hiding this comment

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

Per PEP 484:

Modules and variables imported into the stub are not considered exported from the stub unless the import uses the import ... as ... form or the equivalent from ... import ... as ... form. (UPDATE: To clarify, the intention here is that only names imported using the form X as X will be exported, i.e. the name before and after as must be the same.)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I probably asked this last time I looked at this code.

@mattip mattip merged commit 98cb6bc into numpy:master Feb 24, 2021
@mattip
Copy link
Member

mattip commented Feb 24, 2021

Thanks @bashtage

@BvB93 BvB93 mentioned this pull request Feb 24, 2021
Closed
@bashtage bashtage deleted the randomstate-typing branch April 21, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattip mattip mattip left review comments

@BvB93 BvB93 BvB93 approved these changes

Assignees
No one assigned
Labels
01 - Enhancement Static typing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bashtage @BvB93 @mattip