Skip to content

BUG: fix mgrid output for lower precision float inputs#16815

Merged
mattip merged 5 commits intonumpy:masterfrom
cjblocker:mgrid-float
Jul 25, 2020
Merged

BUG: fix mgrid output for lower precision float inputs#16815
mattip merged 5 commits intonumpy:masterfrom
cjblocker:mgrid-float

Conversation

@cjblocker
Copy link
Contributor

@cjblocker cjblocker commented Jul 12, 2020

Floats besides float64 were being coerced to integers in np.mgrid and complex step sizes for the index trick classes would fail for complex64 input. Fixes #16466

Before:

>>> np.mgrid[np.float32(0.1):np.float32(0.33):np.float32(0.1),]
array([[0, 0, 0]])
>>> np.r_[0:10:np.complex64(3j)]
array([], dtype=complex128)

After:

>>> np.mgrid[np.float32(0.1):np.float32(0.33):np.float32(0.1),]
array([[0.1, 0.2, 0.3]])
>>> np.r_[0:10:np.complex64(3j)]
array([ 0.,  5., 10.])

The input types do not affect the output dtype, which are always int64 or float64, but they can affect the output in other ways due to arange precision issues (#5808).

>>> np.mgrid[np.float32(0.1):np.float32(0.3):np.float32(0.1)]
array([0.1, 0.2, 0.3])
>>> np.mgrid[np.float64(0.1):np.float64(0.3):np.float64(0.1)]
array([0.1, 0.2])

@seberg mentioned that the mgrid code could use a refactor. A lot of its functionality is duplicated in AxisConcatenator and meshgrid. I think the code could almost be replaced by np.meshgrid(*[np.r_[k.start:k.stop:k.step] for k in key], sparse=self.sparse). I could verify this and clean it up if it sounds reasonable.

Floats besides float64 were being coerced to integers
and complex step sizes for the index trick classes
would fail for complex64 input. Fixes numpy#16466
@charris charris closed this Jul 12, 2020
@charris charris reopened this Jul 12, 2020
@cjblocker
Copy link
Contributor Author

Let me know if there is anything else or if I need to squash the smaller commits, I'm not sure what NumPy's preference is.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @cjblocker !

@cjblocker cjblocker requested a review from eric-wieser July 24, 2020 06:04
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me too - I still wonder if we want to produce a float32 result for some of these cases, but that can be a follow-up.

@mattip
Copy link
Member

mattip commented Jul 24, 2020

Since this is a pretty visible bug-fix, do we want to mention it in the release notes?

@cjblocker
Copy link
Contributor Author

@mattip Seems reasonable, I went ahead and wrote a release note.

@mattip mattip merged commit f457a1a into numpy:master Jul 25, 2020
@mattip
Copy link
Member

mattip commented Jul 25, 2020

Thanks @cjblocker and reviewers

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.

BUG: mgrid result sensitive to data type

5 participants

Comments