Skip to content

Conversation

@shlff
Copy link
Member

@shlff shlff commented Jul 18, 2021

Hi @jstac and @oyamad , thanks for your nice comments. This PR fix #165 's remaining patterns, mentioned by @oyamad , except for the following:

Do you think we should also change the np.ones pattern to np.full in the above lecture?

@github-actions github-actions bot temporarily deployed to commit July 18, 2021 08:10 Inactive
I = np.identity(n)
v = solve(I - β * K, β * K @ np.ones(n))
v = solve(I - β * K, K @ np.full(n, β))
Copy link
Contributor

Choose a reason for hiding this comment

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

@shlff , please revert this back to the original, since the original is a bit closer to the maths. Thanks.

I = np.identity(ap.n)
Ones = np.ones(ap.n)
v = solve(I - β * J, β * J @ Ones)
Full = np.full(ap.n, β)
Copy link
Contributor

Choose a reason for hiding this comment

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

@shlff , please revert this back to the original. Ones is a natural matrix and the code is easier to read in the original.

I = np.identity(ap.n)
Ones = np.ones(ap.n)
p = solve(I - β * M, β * ζ * M @ Ones)
Full = np.full(ap.n, β * ζ)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

π = np.pi
zeros = np.zeros
ones = np.ones
full = np.full
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good strategy. One can use just np.full in line 140 (only one place).

Same for zeros.

@github-actions github-actions bot temporarily deployed to commit July 18, 2021 10:01 Inactive
@shlff
Copy link
Member Author

shlff commented Jul 18, 2021

Thanks for your nice comments @jstac and @oyamad .

I fix the PR with all your existing concerns resolved.

Looking forward to your further comments. :)

@github-actions github-actions bot temporarily deployed to commit July 18, 2021 10:16 Inactive
Copy link
Member

@oyamad oyamad left a comment

Choose a reason for hiding this comment

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

@shlff Good work!

@jstac
Copy link
Contributor

jstac commented Jul 18, 2021

Perfect, many thanks @shlff and @oyamad . Merging.

@jstac jstac merged commit 2abf38b into main Jul 18, 2021
@jstac jstac deleted the pattern_change branch July 18, 2021 18:30
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.

Use np.full?

4 participants