numpy / numpy-stubs

Experimental typing stubs for NumPy
BSD 3-Clause "New" or "Revised" License
282 stars 32 forks source link

Add NumPy scalar hierarchy #14

Closed alanhdu closed 6 years ago

alanhdu commented 6 years ago

I've factored out an _ArrayLike class (although I need to play around with the types some more to see how to get it to typecheck the way I want).

I've also moved the platform-specific types together. AFAICT though, on the Python side the platform-specific types are aliases for the sized types (e.g. for me, np.short.__name__ == 'int16'), so the problem reduces to how to decide which sized type the unsized types correspond to (mypy supports sys.platform and maybe also sys.maxsize?). Or is that not how it actually works?

eric-wieser commented 6 years ago

on the Python side the platform-specific types are aliases for the sized types

The __name__ attributes are misleading and currently non-unique (numpy/numpy#10151)

>>> np.int_.__name__  # on windows, this gives int32, so use `intc` below
'int64'
>>> np.longlong.__name__
'int64'
>>> np.int_ is np.longlong  # C long and C longlong are different types
False

There really is a unique type for each of the underlying C types, but the python-visible names are the sized name for that type. Does mypy use the stubs to print the type name in a message, or the actual __name__ attribute?

alanhdu commented 6 years ago

I believe that MyPy uses the actual type name (from a small test case):

Argument 1 to "f" has incompatible type "int64"; expected "longlong"

Is the "right" thing to do here is to just to have each of the platform specific types be their own distinct type? How important is it to get all the sized aliases matched up with their underlying components and how hard is that to do?

JelleZijlstra commented 6 years ago

Mypy definitely doesn't look at the runtime __name__ attribute; all it knows about numpy types necessarily comes from the stubs.

eric-wieser commented 6 years ago

As part of setup.py, we could import numpy and generate the alias stubs based on the actual alias values

alanhdu commented 6 years ago

I really like generating the alias stubs when the user installs them! I've opened up a follow-up issue (#15) to discuss that idea further.

For now, I've fixed the type errors and decided to leave the platform-specific types for another PR (which by default leaves them as Any types). Do you mind taking another look @shoyer @eric-wieser?

(Also sorry for the super long iteration cycles -- I unfortunately don't have a ton of time to really zero-in on this so it usually takes a couple days for me to circle back to stubbing out numpy).

shoyer commented 6 years ago

👍 this looks great to me!

Two things that would be nice to have:

eric-wieser commented 6 years ago

Update the return value of dtype.type to Type[generic].

What about object?

Edit: nevermind

alanhdu commented 6 years ago

@shoyer @eric-wieser I went a little overboard with the test suite and ended up writing a small test framework (I wanted to assert that certain things failed and that led me down a bit of a rabbit hole).

I put the documentation in the testing readme, the important bit is that:

There are three main directories of tests right now:

There are three main directories of tests right now:

  • pass/ which contain Python files that must pass mypy checking with no type errors
  • fail/ which contain Python files that must fail mypy checking with the annotated errors
  • reveal/ which contain Python files that must output the correct types with reveal_type

fail and reveal are annotated with comments that specify what error mypy threw and what type should be revealed respectively. The format looks like:

bad_function   # E: <error message>
reveal_type(x)   # E: <type name>

Right now, the error messages and types are must be contained within corresponding mypy message.

Running the tests

We use py.test to orchestrate our tests. You can just run:

py.test

to run the entire test suite. To run mypy on a specific file (which can be useful for debugging), you can also run:

$ cd tests
$ MYPYPATH=.. mypy <file_path>

Is that ok? If not, I'm happy to move the test suite stuff to another PR for a longer discussion (I'm not totally sold on the # E: annotations and I'm not super happy with having to parse mypy's string output).

shoyer commented 6 years ago

The test suite bit looks pretty sweet to me! Thanks @alanhdu

alanhdu commented 6 years ago

@shoyer @eric-wieser Do you mind taking another look at this? Hopefully it's close :laughing:.

shoyer commented 6 years ago

OK, looks good to me.

@eric-wieser any more thoughts before I merge?

shoyer commented 6 years ago

OK, merged. Thanks @alanhdu