Skip to content
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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Renelvon
Copy link

@Renelvon Renelvon commented Mar 14, 2020

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.

@rougier
Copy link
Owner

rougier commented Mar 18, 2020

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).

@Renelvon
Copy link
Author

I only updated the .ktx file manually, the rest was done by running the generator in the last commit. Should I only submit changes in the .ktx file? I will close the PR and resubmit one PR per fix once you clarify. :)

@@ -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))
Copy link
Owner

Choose a reason for hiding this comment

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

Why this change ?

Copy link
Author

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.

Copy link
Contributor

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)
Copy link
Owner

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)

Copy link
Author

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?

Copy link
Owner

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)
Copy link
Owner

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)

Copy link
Author

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.

Copy link
Owner

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))
Copy link
Owner

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 ?

Copy link
Author

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

Copy link
Owner

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
Copy link
Owner

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 ?

Copy link
Owner

Choose a reason for hiding this comment

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

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No.9's solution is wrong in 100_Numpy_exercises_with_hints_with_solutions.md file
3 participants