Skip to content

Fix to preserve dtype of input array in cupy.linalg.norm#875

Merged
okuta merged 3 commits intocupy:masterfrom
kmaehashi:numpy-1.14-norm
May 11, 2018
Merged

Fix to preserve dtype of input array in cupy.linalg.norm#875
okuta merged 3 commits intocupy:masterfrom
kmaehashi:numpy-1.14-norm

Conversation

@kmaehashi
Copy link
Copy Markdown
Member

@kmaehashi kmaehashi commented Jan 11, 2018 •
edited
Loading

In NumPy 1.14, np.linalg.norm has been changed to preserve dtype of input array.
However, as fix made in NumPy 1.14 was incomplete, there are some cases (e.g., when arbitrary order is specified) that dtype is not preserved.
The issue is recognized by the team and the additional fix is ongoing.

This PR makes CuPy behavior consistent with NumPy 1.14.1 (assuming the above PR got merged and released).

We need to discuss if we should separate tests for <1.14, ==1.14, '>1.14'.
(Maybe we can ignore ==1.14 ... but in that case, can we say that we support NumPy 1.14?)

@kmaehashi kmaehashi changed the title [WIP] fix to preserve dtype of input array in cupy.linalg.norm [RFC] fix to preserve dtype of input array in cupy.linalg.norm Jan 11, 2018
@okuta okuta added to-be-backported Pull-requests to be backported to stable branch cat:numpy-compat Follow the NumPy/SciPy spec changes labels Jan 15, 2018
@kmaehashi kmaehashi changed the title [RFC] fix to preserve dtype of input array in cupy.linalg.norm [WIP] fix to preserve dtype of input array in cupy.linalg.norm Jan 17, 2018
@okuta okuta assigned okuta and unassigned bkvogel Feb 18, 2018
@okuta
Copy link
Copy Markdown
Member

okuta commented Feb 19, 2018

I think that CuPy aims at the same behavior as the latest NumPy

eric-wieser
eric-wieser reviewed Feb 26, 2018
View reviewed changes
cupy/linalg/norms.py
absx **= ord
ret = absx.sum(axis=axis, keepdims=keepdims)
ret **= (1.0 / ord)
ret **= cupy.reciprocal(ord, dtype=ret.dtype)
Copy link
Copy Markdown

@eric-wieser eric-wieser Feb 26, 2018

Choose a reason for hiding this comment

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

I think you need to update this - np.reciprocal fails if ord is an integer, so yours might too

Copy link
Copy Markdown
Member Author

@kmaehashi kmaehashi Feb 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the comment!
I guess both NumPy and CuPy work correctly if dtype is given to reciprocal?

>>> import numpy as np, cupy as cp
>>> np.reciprocal(10, dtype=np.float32)
0.1
>>> cp.reciprocal(10, dtype=cp.float32)
array(0.1, dtype=float32)

Copy link
Copy Markdown
Member Author

@kmaehashi kmaehashi Feb 26, 2018

Choose a reason for hiding this comment

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

I raised a PR regarding this, could you please confirm? numpy/numpy#10667

@kmaehashi kmaehashi mentioned this pull request Feb 26, 2018
Closed
okuta
okuta reviewed Mar 1, 2018
View reviewed changes
cupy/linalg/norms.py Outdated
# Zero norm
# Convert to Python float in accordance with NumPy
return (x != 0).sum(axis=axis, keepdims=keepdims, dtype='d')
return (x != 0).astype(x.real.dtype).sum(axis=axis, keepdims=keepdims)
Copy link
Copy Markdown
Member

@okuta okuta Mar 1, 2018

Choose a reason for hiding this comment

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

Cloud you fix this line.
./cupy/linalg/norms.py:72:80: E501 line too long (82 > 79 characters)

Copy link
Copy Markdown
Member Author

@kmaehashi kmaehashi Mar 15, 2018

Choose a reason for hiding this comment

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

Fixed.

@okuta okuta added this to the v4.0.0rc1 milestone Mar 15, 2018
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 15, 2018 •
edited
Loading

Codecov Report

Merging #875 into master will decrease coverage by 3.95%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #875      +/-   ##
==========================================
- Coverage   93.23%   89.27%   -3.96%     
==========================================
  Files         107      107              
  Lines        5984     5848     -136     
==========================================
- Hits         5579     5221     -358     
- Misses        405      627     +222
Impacted Files Coverage Δ
cupy/linalg/norms.py 42.46% <0%> (-49.04%) ⬇️
cupy/fft/fft.py 18.42% <0%> (-80.93%) ⬇️
cupy/binary/packing.py 56.25% <0%> (-31.25%) ⬇️
cupy/manipulation/rearrange.py 82.19% <0%> (-15.07%) ⬇️
cupy/creation/matrix.py 86.66% <0%> (-6.67%) ⬇️
cupy/padding/pad.py 83.05% <0%> (-5.66%) ⬇️
cupy/sparse/csr.py 81.95% <0%> (-3.76%) ⬇️
cupy/cuda/memory_hooks/line_profile.py 95.23% <0%> (-3.58%) ⬇️
cupy/sparse/dia.py 92.53% <0%> (-2.99%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b082c98...6fd3106. Read the comment docs.

@okuta
Copy link
Copy Markdown
Member

okuta commented Mar 19, 2018

I got error on NumPy 1.14.2 environment.
Cloud you fix this?

@okuta okuta added the st:awaiting-author Awaiting response from author label Mar 19, 2018
@hvy hvy removed this from the v4.0.0rc1 milestone Mar 20, 2018
@kmaehashi kmaehashi added to-be-backported Pull-requests to be backported to stable branch and removed to-be-backported Pull-requests to be backported to stable branch labels Mar 20, 2018
@kmaehashi
Copy link
Copy Markdown
Member Author

kmaehashi commented Apr 13, 2018 •
edited
Loading

I fixed the test code to use type_check=False for 1-D norm tests.
This is due to a NumPy bug and will be fixed in numpy/numpy#10667.
Once the PR got merged, we can remove type_check=False.

(The root cause of this issue is that NumPy started to use x **= in NumPy 1.14 (https://github.com/numpy/numpy/pull/10390/files#diff-6858bbac5c5c59bdddf245c69d6a547cR2296), but it's type promotion rule is different depending on whether x is a scalar or ndarray.)

@kmaehashi kmaehashi changed the title [WIP] fix to preserve dtype of input array in cupy.linalg.norm Fix to preserve dtype of input array in cupy.linalg.norm Apr 13, 2018
@kmaehashi kmaehashi mentioned this pull request Apr 13, 2018
Merged
@okuta okuta added this to the v5.0.0a1 milestone Apr 16, 2018
@kmaehashi kmaehashi force-pushed the numpy-1.14-norm branch 3 times, most recently from 1fe93f3 to 4cf6c8d Compare April 16, 2018 09:01
@hvy hvy modified the milestones: v5.0.0a1, v5.0.0b1 Apr 17, 2018
@kmaehashi
Copy link
Copy Markdown
Member Author

kmaehashi commented May 8, 2018

Resolved conflicts.

@okuta okuta removed the st:awaiting-author Awaiting response from author label May 11, 2018
@okuta
Copy link
Copy Markdown
Member

okuta commented May 11, 2018

LGTM!

@okuta okuta merged commit 6292c18 into cupy:master May 11, 2018
@okuta okuta removed the to-be-backported Pull-requests to be backported to stable branch label May 11, 2018
@kmaehashi kmaehashi deleted the numpy-1.14-norm branch May 11, 2018 02:44
@kmaehashi kmaehashi added the cat:enhancement Improvements to existing features label May 23, 2018
@okuta okuta added the to-be-backported Pull-requests to be backported to stable branch label Jun 17, 2018
okuta added a commit to okuta/cupy that referenced this pull request Jun 17, 2018
@okuta
Merge pull request cupy#875 from kmaehashi/numpy-1.14-norm
a605ce1 
Fix to preserve dtype of input array in cupy.linalg.norm
@okuta okuta mentioned this pull request Jun 17, 2018
Merged
This was referenced Nov 4, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@okuta okuta okuta left review comments

+1 more reviewer

@eric-wieser eric-wieser eric-wieser left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@okuta okuta

Labels

cat:enhancement Improvements to existing features cat:numpy-compat Follow the NumPy/SciPy spec changes to-be-backported Pull-requests to be backported to stable branch

Projects

None yet

Milestone

v5.0.0b1

Development

Successfully merging this pull request may close these issues.

6 participants

@kmaehashi @okuta @codecov-io @eric-wieser @hvy @bkvogel