Skip to content

Suggestions for the SVD lecture #361

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

Merged
merged 8 commits into from
Dec 23, 2023
Merged

Suggestions for the SVD lecture #361

merged 8 commits into from
Dec 23, 2023

Conversation

matheusvillasb
Copy link
Contributor

Corrected some typos.

Added a brief description explaining why the Eckart-Young theorem is important.

Added details to how the PCA for any given matrix.

Changed the n_components term in the codes to r_components, so it is in accord with the definition of the Eckward-Young theorem and doesn't create confusion with the n columns of the matrix.

Added an exercise and its solution at the end (I've never used Sphinx, so not sure if it's done correctly).

…kart-Young theorem is important.

Added details to how the PCA for any given matrix.

Changed the n_components term in the codes to r_components, so it is in accord with the definition of the Eckward-Young theorem and doesn't create confusion with the n columns of the matrix.

Added an exercise and its solution at the end (I've never used sphinx, so not sure if it's done correctly).
@jstac
Copy link
Contributor

jstac commented Aug 16, 2023

Many thanks @matheusvillasb for your very helpful corrections and suggestions!

@HumphreyYang , could you please do a first round review of this PR? @matheusvillasb is a first-time contributor, and hence might need some assistance with QE conventions and myst syntax.

@thomassargent30, just looping you into this discussion since the edits are to your SVD lecture. @matheusvillasb is a very enthusiastic and hard working masters student based in Europe.

@thomassargent30
Copy link
Contributor

thomassargent30 commented Aug 16, 2023 via email

Copy link
Collaborator

@HumphreyYang HumphreyYang left a comment

Choose a reason for hiding this comment

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

Great changes @matheusvillasb, many thanks.

Please kindly check my comments below and feel free to commit them. I will do another round in a separate PR once these changes are made.

matheusvillasb and others added 3 commits August 17, 2023 17:15
Removed typos and excessive lines that were pointed out by HumphreyYang
just being sure I commited all changes
@matheusvillasb
Copy link
Contributor Author

matheusvillasb commented Aug 17, 2023 via email

@HumphreyYang
Copy link
Collaborator

HumphreyYang commented Aug 17, 2023

Hi Matheus,

so singular values is more appropriate.

You are absolutely right. That was a typo from me.

I'm sorry if I did anything wrong, I'm still figuring out how to do it.

No, you are doing great. Thanks for the great contribution.

Please feel free to go ahead with the DMD lecture in a separate PR. Please do not hesitate to let me know if you need any help.

Thanks,
Humphrey

@@ -360,8 +360,13 @@ $$
\hat X_r = \sigma_1 U_1 V_1^\top + \sigma_2 U_2 V_2^\top + \cdots + \sigma_r U_r V_r^\top
$$ (eq:Ekart)

This is a very powerful theorem, it says that we can take our $ m \times n $ matrix $X$ that in not full rank, and we can best approximate it to a full rank $p \times p$ matrix through the SVD.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please changes as follows

"...powerful theorem that says..."
"...approximate it by a..."

You can read about the Eckart-Young theorem and some of its uses here <https://en.wikipedia.org/wiki/Low-rank_approximation>.
Moreover, if some of these $p$ singular values carry more information than others, and if we want to have the most amount of information with the least amount of data, we can take $r$ leading singular values ordered by magnitude.

But more about it later when we present Principal Component Analysis.
Copy link
Contributor

Choose a reason for hiding this comment

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

"We'll say more about this later when..."


In Ordinary Least Squares (OLS), we learn to compute $ \hat{\beta} = (X^\top X)^{-1} X^\top y $, but there are cases such as when we have colinearity or an underdetermined system: **short fat** matrix.

In these cases, the $ (X^\top X) $ matrix is not inversible. Its determinant is zero or close to zero and we cannot invert it.
Copy link
Contributor

Choose a reason for hiding this comment

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

"...not invertible (its determinant is zero) or ill-conditioned (its determinant is very close to zero)."

@jstac
Copy link
Contributor

jstac commented Aug 18, 2023

Thanks @matheusvillasb , these are nice changes. Thoughtful and well written.

I've requested some very minor edits. Would you mind to make those changes and push them to this PR?

Once you have made those edits to the PR I'll pass the review over to @mmcky and @thomassargent30 so we can get this merged.

(Thanks also to @HumphreyYang for a very useful review.)

Copy link
Contributor Author

@matheusvillasb matheusvillasb left a comment

Choose a reason for hiding this comment

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

Did the changes John asked, sorry for the delay, I thought I had commited it before, but it didn't.

@jstac
Copy link
Contributor

jstac commented Aug 20, 2023

Great, thanks @matheusvillasb . Over to you @mmcky and @thomassargent30 .

corrected definition of pseudo inverse of sigma matrix in the exercise
@jstac
Copy link
Contributor

jstac commented Sep 1, 2023

@HumphreyYang , could you please look and see why this is failing?

@HumphreyYang
Copy link
Collaborator

HumphreyYang commented Sep 1, 2023

@HumphreyYang , could you please look and see why this is failing?

Hi @jstac,

I think @mmcky raised this issue of GitHub action. It rejects running PR from a fork as it is reluctant to share the credentials for EC2. I think we have yet to find a good solution other than building it locally (CC @mmcky).

@mmcky
Copy link
Contributor

mmcky commented Dec 12, 2023

@HumphreyYang can you run this locally and cross-check the builds and report back.

It would be great if you can also fix the merge conflict.

@mmcky
Copy link
Contributor

mmcky commented Dec 12, 2023

@HumphreyYang can you run this locally and cross-check the builds and report back.

It would be great if you can also fix the merge conflict.

Alternatively please transfer the PR to a local branch and push (giving @matheusvillasb the credit to get CI to pass)

@mmcky
Copy link
Contributor

mmcky commented Dec 13, 2023

Closing this in preference for #375 (migrated to a local branch)

@mmcky mmcky closed this Dec 13, 2023
@mmcky mmcky reopened this Dec 14, 2023
@mmcky
Copy link
Contributor

mmcky commented Dec 14, 2023

I have re-opened this as we can use #375 as a test environment so long as we move those changes back to this PR (that can't execute the previews). @HumphreyYang makes a good point in that we want attribution to be retained for @matheusvillasb so it will be better to merge this into main than #375.

@HumphreyYang can you use gh cli to

gh checkout pr matheusvillasb:main
<resolve conflict>
git merge main
git push

to bring this PR (fork in line with #375)

@mmcky
Copy link
Contributor

mmcky commented Dec 14, 2023

@HumphreyYang I see the fix 0770ded and I can apply this if you're busy today. 👍

@mmcky
Copy link
Contributor

mmcky commented Dec 14, 2023

@jstac a replica of this is available here: #375

and a preview

https://65782eb0f8c9513bb97425e7--nostalgic-wright-5fa355.netlify.app/svd_intro.html

@mmcky mmcky added the review label Dec 19, 2023
@mmcky mmcky mentioned this pull request Dec 19, 2023
@mmcky mmcky requested review from HumphreyYang and jstac December 19, 2023 22:32
@jstac
Copy link
Contributor

jstac commented Dec 19, 2023

Hi @mmcky, thanks for organizing. Thanks @matheusvillasb for putting this together and @HumphreyYang for the review.

I'm happy with these changes but they should be approved by @thomassargent30 before being made live.

@mmcky
Copy link
Contributor

mmcky commented Dec 23, 2023

thanks @matheusvillasb for these proposed changes.

Thanks @HumphreyYang @jstac for your comments and reviews.

@thomassargent30 has reviewed and will make a few minor changes once merged into the main branch.

@mmcky mmcky merged commit 3dbdccd into QuantEcon:main Dec 23, 2023
@mmcky
Copy link
Contributor

mmcky commented Jan 1, 2024

thanks once again @matheusvillasb -- these changes are going live today.

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

Successfully merging this pull request may close these issues.

5 participants