numpy / numpy-stubs

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

Unsigned int constructors too generous #72

Closed person142 closed 4 years ago

person142 commented 4 years ago

xref https://github.com/numpy/numpy-stubs/pull/70

The code

import numpy as np

class A:
    def __float__(self):
        return 4.0

np.uint8(A())

fails at runtime, but passes type checking. The reason is here:

https://github.com/numpy/numpy-stubs/blob/master/numpy-stubs/__init__.pyi#L402

where the constructor of number is:

class number(generic):
    def __init__(self, value: Union[SupportsInt, SupportsFloat] = ...) -> None: ...
rajatdiptabiswas commented 4 years ago

Would you kindly let me know what exactly needs to be done so that I can try working on the issue?

person142 commented 4 years ago

Hey @rajatdiptabiswas, so what will need to be done is adding more specific constructors for things in the signed integer hierarchy:

https://github.com/numpy/numpy-stubs/blob/master/numpy-stubs/__init__.pyi#L420

and the unsigned integer hierarchy:

https://github.com/numpy/numpy-stubs/blob/master/numpy-stubs/__init__.pyi#L444

The constructors will be similar to the generic one here:

https://github.com/numpy/numpy-stubs/blob/master/numpy-stubs/__init__.pyi#L402

but will not longer include SupportsFloat. And of course tests will need to be added!

rajatdiptabiswas commented 4 years ago

Thanks for the clarification @person142 I have an exam coming up but I shall try my best to get it done as soon as possible!

rajatdiptabiswas commented 4 years ago

Hi @person142

I have tried looking into the issue and these are the things I believe I should be doing.

For each of the classes int8, int16, int32, int64, uint8, uint16, uint32, and uint64 in numpy-stubs/core/__init__.pyi I should create constructors similar to the one in class number(generic) and remove SupportsFloat from the definition.

Hence each of the int8, ..., uint64 functions will be updated to something like:

class int64(signedinteger): 
    def __init__(self, value: SupportsInt = ...) -> None: ...

class uint64(unsignedinteger): 
    def __init__(self, value: SupportsInt = ...) -> None: ...

instead of the current implementation

class int64(signedinteger): ...

class uint64(unsignedinteger): ...

Would you kindly confirm if these are correct?

I also needed some help writing the tests. I believe I should edit the file numpy-stubs/tests/pass/scalars.py. However, I can't figure out exactly how or what to write.

I am new to writing tests and anything that might help me learn and complete this task would be amazing.

I shall start working on these as soon as you confirm. Thanks in advance!

person142 commented 4 years ago

Would you kindly confirm if these are correct?

Yes, that looks correct.

I also needed some help writing the tests. I believe I should edit the file numpy-stubs/tests/pass/scalars.py. However, I can't figure out exactly how or what to write.

Since we are allowing less in the constructor, I would add cases to tests/fail/scalars.py (as more things should fail now). That would probably look something like

class A:
    def __float__(self):
        return 1.0

np.uint8(A())  # E: <whatever-error-mypy-gives>
...

Note that you can run the tests by doing python runtests.py from the repo root.

rajatdiptabiswas commented 4 years ago

I have made the changes you mentioned @person142.

Kindly take a look as to whether I have made them correctly. If you find something wrong please let me know. I'll make the necessary updates!

These are the changes that I have made:

rajatdiptabiswas commented 4 years ago

Looks like the checks have failed for the PR. I'll look into what's wrong.