Skip to content

BUG: Remove complex floor divide#19135

Merged
charris merged 10 commits intonumpy:mainfrom
ganesh-k13:BUG_13236_remove_complex_floor_divide
Jun 23, 2021
Merged

BUG: Remove complex floor divide#19135
charris merged 10 commits intonumpy:mainfrom
ganesh-k13:BUG_13236_remove_complex_floor_divide

Conversation

@ganesh-k13
Copy link
Copy Markdown
Member

@ganesh-k13 ganesh-k13 commented May 29, 2021 •
edited
Loading

Changes:

  1. Removed floor_divide for all complex types
  2. Modified UFuncs floor_divde TC to check for TypeError. Added same for divmod and remainder.
  3. Modified test_core to check TypeError raised for unsupported types.
  4. Removed Typing Tests

TODO:

  • Add release note (Need PR number for this)

closes: #13236
finishes: #13245 (work done by @kaivu1999)

ganesh-k13
ganesh-k13 commented May 29, 2021
View reviewed changes
numpy/ma/tests/test_core.py
dtype=float_dtype)
assert_equal(zm.min(), float_dtype(-np.inf-1j))
assert_equal(zm.max(), float_dtype(np.inf+2j))
Copy link
Copy Markdown
Member Author

@ganesh-k13 ganesh-k13 May 29, 2021

Choose a reason for hiding this comment

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

I really need to change my .vimrc one day

ganesh-k13 added a commit to ganesh-k13/numpy that referenced this pull request May 29, 2021
@ganesh-k13
DOC: Added doc for TypeError thrown during floor divide (numpy#19135)
32582d8
@ganesh-k13 ganesh-k13 force-pushed the BUG_13236_remove_complex_floor_divide branch from 32582d8 to d4765ee Compare May 29, 2021 18:13
@ganesh-k13
Copy link
Copy Markdown
Member Author

ganesh-k13 commented May 29, 2021 •
edited
Loading

@kaivu1999 didn't cross my mind to check with you before raising this. If you see this before the review for this code is done, please feel free to re-open your PR and add the extra changes I have made to fix the tests.

BvB93
BvB93 reviewed May 29, 2021
View reviewed changes
Copy link
Copy Markdown
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.

There are a couple of places in the stub files that still contain references to complex floordivision:

numpy/numpy/__init__.pyi

Lines 2310 to 2311 in 9fcc132

@overload
def __floordiv__(self: _ArrayComplex_co, other: _ArrayLikeComplex_co) -> NDArray[complexfloating[Any, Any]]: ... # type: ignore[misc]

numpy/numpy/__init__.pyi

Lines 2338 to 2339 in 9fcc132

@overload
def __rfloordiv__(self: _ArrayComplex_co, other: _ArrayLikeComplex_co) -> NDArray[complexfloating[Any, Any]]: ... # type: ignore[misc]

numpy/numpy/__init__.pyi

Lines 3258 to 3259 in 9fcc132

__floordiv__: _ComplexOp[_NBit1]
__rfloordiv__: _ComplexOp[_NBit1]

The same holds for the typing tests in reveal/arithmetic.py and pass/arithmetic.py.

@ganesh-k13
Copy link
Copy Markdown
Member Author

ganesh-k13 commented May 30, 2021

Thanks @BvB93, I'll remove those as well. I wanted to understand how and where the three references in numpy/numpy/__init__.pyi are being used. When I tried removing them for unsigned int, I was still able to go forward with the division. So where are they used?

ganesh-k13
ganesh-k13 commented May 30, 2021
View reviewed changes
@BvB93
Copy link
Copy Markdown
Member

BvB93 commented May 30, 2021

When I tried removing them for unsigned int, I was still able to go forward with the division. So where are they used?

Yeah, that makes sense actually. Each of these _ArrayLike<x>_co variables represents an union of all types that can be cast into <x>, according to the rules of casting="same_kind". So removing the overload for unsigned integers means that, next time, they'll be handled by the overload for signed integers. Remove that overload as well and they'll be handled by the float overload, etc..

Complex is at the end of this tree though (as it cannot be safely cast into any other type), so removing it means that complex operations are no longer allowed.

numpy/numpy/__init__.pyi

Lines 2016 to 2021 in 9fcc132

@overload
def __matmul__(self: _ArrayUInt_co, other: _ArrayLikeUInt_co) -> NDArray[unsignedinteger[Any]]: ... # type: ignore[misc]
@overload
def __matmul__(self: _ArrayInt_co, other: _ArrayLikeInt_co) -> NDArray[signedinteger[Any]]: ... # type: ignore[misc]
@overload
def __matmul__(self: _ArrayFloat_co, other: _ArrayLikeFloat_co) -> NDArray[floating[Any]]: ... # type: ignore[misc]

@ganesh-k13
Copy link
Copy Markdown
Member Author

ganesh-k13 commented May 30, 2021

Thanks! Always wondered where the casting order is taken care for arrays.

BvB93
BvB93 reviewed May 30, 2021
View reviewed changes
@ganesh-k13 ganesh-k13 force-pushed the BUG_13236_remove_complex_floor_divide branch from 672fb9f to f59e225 Compare May 30, 2021 17:49
@ganesh-k13
Copy link
Copy Markdown
Member Author

ganesh-k13 commented Jun 10, 2021

Hey folks, any changes needed here?

mattip
mattip approved these changes Jun 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

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

Do we know if this will have implications for scipy/astropy/pandas?

@ganesh-k13
Copy link
Copy Markdown
Member Author

ganesh-k13 commented Jun 11, 2021

Will building them with this branch of NumPy help? Will be happy to do so.

@charris charris merged commit a79a913 into numpy:main Jun 23, 2021
@charris
Copy link
Copy Markdown
Member

charris commented Jun 23, 2021

Thanks @ganesh-k13 .Lets get this in for testing at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BvB93 BvB93 BvB93 left review comments

@mattip mattip mattip approved these changes

Assignees

No one assigned

Labels

00 - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

np.floor_divide drops imaginary part silently

4 participants

@ganesh-k13 @BvB93 @charris @mattip