You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I was reading through the eig_circulant lecture and had a few small suggestions.
I’d be grateful if you could take a look when you have a chance.
Numba code
At lines 34, 69, and 203, the lecture uses Numba only for two small illustrative helper functions. Since these functions are not benchmarked and performance is not part of the discussion here, it may be clearer to remove the @jit decorators and the corresponding from numba import jit import, keeping the examples focused on circulant matrices and Fourier structure.
Circulant matrices and convolution
At line 93, it may be helpful to clarify that, when discussing the properties of sums and products of circulant matrices, the matrices use the same cyclic-shift convention.
At line 114, the convolution index c_{k-i} should probably be described as being taken modulo N. Otherwise, k-i can be negative.
At line 114, lowercase n appears in the sum after the lecture has introduced an N x N circulant matrix. It may be clearer to use N consistently.
At lines 121 and 123, the transpose notation could be updated from C^T to C^\top, matching the style guide.
Permutation matrix connection
At line 179, the determinant formula for the cyclic shift matrix seems to be missing parentheses. It currently reads as
At line 192, the transpose notation could be updated from P P' = I to P P^\top = Ifollowing the style guide
Python examples
At line 203, the small helper constructor probably does not need @jit. Since it is mainly illustrative, removing @jit may make the example simpler.
At line 204, the function name construct_P is a bit terse. Something like construct_cyclic_shift_matrix might be more informative.
At line 227, in the np.linalg.eig example, eigenvectors are stored in the columns of Q, so the displayed eigenvector associated with λ[i] should be Q[:, i] rather than Q[i, :].
Current code:
print(f'𝜆{i} = {𝜆[i]:.1f}\nvec{i} = {Q[i, :]}\n')
At line 290, “Discete Fourier Transform” should be “Discrete Fourier Transform.”
Associated permutation matrix
At line 356, lowercase n appears in c_{n-1} P^{n-1}. It may be clearer to use N consistently.
At line 361, there is an empty code cell that can probably be removed.
At line 376, the function name construct_cirlulant has a typo and should be construct_circulant.
At line 406, the sentence describing eigenvectors of the circulant matrix says “the kth column of P_8”, but I think it should refer to a column of Q_8 or F_8 as P_8 is the cyclic shift matrix.
Current text:
The kth column of P_8 associated with eigenvalue w^{k-1} is an eigenvector of C_8...
At line 406, the eigenvalue formula in the same sentence appears to have mismatched indices. Currently the text say: $\sum_{h=0}^{7} c_{j} w^{h k}$, where the summation index should be $k$ instead of $h$.
Discrete Fourier transform
At line 433, “a sequence of N real number” should be “a sequence of N real numbers.”
At line 501, the plot label for DFT coefficients is j, but the DFT formula uses X_k. It may be clearer to label the frequency index as k.
At line 529, inverse_transform could be renamed to inverse_DFT or IDFT to match the earlier DFT function.
At line 554, “availble” should be “available.”
At line 617, “tranform” should be “transform.”
At line 627, “a inverse” should be “an inverse.”
At line 634, there is an empty code cell that can probably be removed.
Hi @jstac
I was reading through the
eig_circulantlecture and had a few small suggestions.I’d be grateful if you could take a look when you have a chance.
Numba code
@jitdecorators and the correspondingfrom numba import jitimport, keeping the examples focused on circulant matrices and Fourier structure.Circulant matrices and convolution
At line 93, it may be helpful to clarify that, when discussing the properties of sums and products of circulant matrices, the matrices use the same cyclic-shift convention.
At line 114, the convolution index
c_{k-i}should probably be described as being taken moduloN. Otherwise,k-ican be negative.At line 114, lowercase
nappears in the sum after the lecture has introduced anN x Ncirculant matrix. It may be clearer to useNconsistently.At lines 121 and 123, the transpose notation could be updated from
C^TtoC^\top, matching the style guide.Permutation matrix connection
At line 179, the determinant formula for the cyclic shift matrix seems to be missing parentheses. It currently reads as
I think this should be
so that the eigenvalues solve
At line 192, the transpose notation could be updated from
P P' = ItoP P^\top = Ifollowing the style guidePython examples
At line 203, the small helper constructor probably does not need
@jit. Since it is mainly illustrative, removing@jitmay make the example simpler.At line 204, the function name
construct_Pis a bit terse. Something likeconstruct_cyclic_shift_matrixmight be more informative.At line 227, in the
np.linalg.eigexample, eigenvectors are stored in the columns ofQ, so the displayed eigenvector associated withλ[i]should beQ[:, i]rather thanQ[i, :].Current code:
At line 290, “Discete Fourier Transform” should be “Discrete Fourier Transform.”
Associated permutation matrix
At line 356, lowercase
nappears inc_{n-1} P^{n-1}. It may be clearer to useNconsistently.At line 361, there is an empty code cell that can probably be removed.
At line 376, the function name
construct_cirlulanthas a typo and should beconstruct_circulant.At line 406, the sentence describing eigenvectors of the circulant matrix says “the
kth column ofP_8”, but I think it should refer to a column ofQ_8orF_8asP_8is the cyclic shift matrix.Current text:
At line 406, the eigenvalue formula in the same sentence appears to have mismatched indices. Currently the text say:$\sum_{h=0}^{7} c_{j} w^{h k}$ , where the summation index should be $k$ instead of $h$ .
Discrete Fourier transform
At line 433, “a sequence of
Nreal number” should be “a sequence ofNreal numbers.”At line 501, the plot label for DFT coefficients is
j, but the DFT formula usesX_k. It may be clearer to label the frequency index ask.At line 529,
inverse_transformcould be renamed toinverse_DFTorIDFTto match the earlierDFTfunction.At line 554, “availble” should be “available.”
At line 617, “tranform” should be “transform.”
At line 627, “a inverse” should be “an inverse.”
At line 634, there is an empty code cell that can probably be removed.
What do you think? Happy to submit a PR.
Best,
Longye