-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes to questions 2, 9, 18, 31, 35, 44, 52, 56, 66 & 78 #106
base: master
Are you sure you want to change the base?
Conversation
* Treating `I` as `np.ubyte` causes multiplication with 256 to overflow. * There is no need to apply `np.unique` to `I.
Thanks. There's a new format where only file has to be changed. Could you use it ? Also, it would be better to have one PR per proposed changed (would make discussion easier). |
I only updated the |
@@ -72,8 +72,8 @@ print(Z) | |||
`hint: reshape` | |||
|
|||
```python | |||
nz = np.nonzero([1,2,0,0,4,0]) | |||
print(nz) | |||
Z = np.arange(9).reshape((3, 3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current solution corresponds to question 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad... Thanks for fixing it!
B = np.ones(3)*2 | ||
C = np.ones(3)*3 | ||
A = np.ones(3) | ||
B = np.full(3, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the version with ones (in order to keep it simple)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will rebase and drop commit. Should the C
matrix stay as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
```python | ||
Z = np.random.random((10,2)) | ||
X,Y = Z[:,0], Z[:,1] | ||
R = np.sqrt(X**2+Y**2) | ||
R = np.hypot(Y,X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Y,X -> X,Y (for consistency)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is such so that it's consistent with the order of arguments in the arctan2
invocation. It's my personal mnemonic, but I will change it if you insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer yes.
@@ -673,10 +673,10 @@ print(F) | |||
# Author: Nadav Horesh | |||
|
|||
w,h = 16,16 | |||
I = np.random.randint(0,2,(h,w,3)).astype(np.ubyte) | |||
I = np.random.randint(0,2,(h,w,3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the cast to ubyte ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a sufficiently long array, the expected number of different "colors" should be 2**3 == 8
. For the solution to work, F
must be a matrix of at least 24-bit integers, but this is not what happens on my machine: I[..., 1] * 256
has type np.uint16
, and the same happens with I[..., 0] * 256 * 256
, leading to overflow. Writing (I[..., 0] * 256) * 256
doesn't solve the problem. A cast to a sufficient wide type has to happen at some point, or the initial cast has to go.
>>> import numpy as np
>>> w, h = 16, 16
>>> I = np.random.randint(0, 2, (h, w, 3))
>>> I2 = I.astype(np.ubyte)
>>> F = I[...,0]*256*256 + I[...,1]*256 +I[...,2]
>>> F2 = I2[...,0]*256*256 + I2[...,1]*256 +I2[...,2]
>>> print(len(np.unique(F)), len(np.unique(F2)))
8 4
>>> print(F.dtype, F2.dtype)
int64 uint16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -826,9 +826,9 @@ np.negative(Z, out=Z) | |||
def distance(P0, P1, p): | |||
T = P1 - P0 | |||
L = (T**2).sum(axis=1) | |||
U = -((P0[:,0]-p[...,0])*T[:,0] + (P0[:,1]-p[...,1])*T[:,1]) / L | |||
U = ((P0 - p)*T).sum(axis=1) / L |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check this is correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
Fixes #98
Fixes #101
I have added a few more fixes and simplifications, let me know what you think.
I ran all the generators; some of the output is due to older commits.