Skip to content

[org_proj] review and update #217

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

[org_proj] review and update #217

wants to merge 23 commits into from

Conversation

longye-tian
Copy link

Hi @jstac Hi @mmcky ,

This PR is to follow up on #216 . In particular, this PR updates

  • Put orthogonal projection theorem and other theorem in the Theorem env and Proof in the Proof env
  • Create links to orthogonal projection theorem
  • Change the latex code fo the first graph of 1.3.2, we have y' and Py' but we don't plot the Py' into blue so it is not really visible from the graph. Maybe we can update the graph to show Py' in blue
  • Fix Typo in the proof of first theorem in 1.4.1, "sufficies" to "suffices"
  • use consistent notation for column vectors

Hi Matt @mmcky , I've got one question about the figure. I've changed the code in latex but not output. Will this be automatically updated or do I need to change the output picture as well?

Best,
Longye

@github-actions github-actions bot temporarily deployed to commit August 4, 2025 02:52 Inactive
@github-actions github-actions bot temporarily deployed to commit August 4, 2025 03:12 Inactive
@github-actions github-actions bot temporarily deployed to commit August 4, 2025 03:39 Inactive
@github-actions github-actions bot temporarily deployed to commit August 4, 2025 03:52 Inactive
@github-actions github-actions bot temporarily deployed to commit August 4, 2025 04:09 Inactive
@mmcky mmcky changed the title Orth proj update [org_proj] review and update Aug 4, 2025
@mmcky
Copy link
Contributor

mmcky commented Aug 4, 2025

Thanks you @longye-tian this is a great PR. I will review in detail tomorrow.

Unless the image is built using a script or by a code-cell it will likely need to be updated in the _static folder.

@mmcky mmcky requested review from Copilot, mmcky and HumphreyYang August 4, 2025 06:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR focuses on improving the mathematical formatting and consistency of the orthogonal projection lecture by converting informal theorem statements to proper theorem environments and fixing various notation and formatting issues.

Key changes include:

  • Converting manual theorem formatting to proper {prf:theorem} and {prf:proof} environments with labels
  • Adding cross-references to theorems using {prf:ref} syntax
  • Fixing a spelling error and improving notation consistency

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
lectures/orth_proj.md Converts theorems to proper environments, adds cross-references, fixes spelling error, and improves notation consistency
lectures/_static/lecture_specific/orth_proj/orth_proj_thm2.tex Updates LaTeX figure code to improve visual representation by adding blue coloring for Py' vector

Copy link
Member

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

Many thanks @longye-tian! Looks really nice!

Just two points after reading the lecture : ) Please feel free to let me know your thoughts.

I pushed some more edits following PEP8 and fixing some typos. The updated figures are also in the latest push.

@@ -594,9 +616,9 @@ Here are some more standard definitions:

> TSS = ESS + SSR
Copy link
Member

Choose a reason for hiding this comment

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

I think a more commonly seen definitions of (centered) TSS, ESS and SSR are given here where the decomposition only holds when $X$ includes intercept term.

So, I think it might be clearer if we replace

Here are some more standard definitions:

* The **total sum of squares** is $:=  \| y \|^2$.
* The **sum of squared residuals** is $:= \| \hat u \|^2$.
* The **explained sum of squares** is $:= \| \hat y \|^2$.

> TSS = ESS + SSR

with

Define:

* The (uncentered) **total sum of squares** (TSS) is $:=  \| y \|^2$.
* The (uncentered) **sum of squared residuals** (SSR) is $:= \| \hat u \|^2$.
* The **explained sum of squares** (ESS) is $:= \| \hat y \|^2$.

 We have the relationship:

$$
\text{TSS} = \text{ESS} + \text{SSR}
$$

```{note}
For the centered case, see [here](https://en.wikipedia.org/wiki/Explained_sum_of_squares).
```

Please let me know your thoughts on this!

Copy link
Author

Choose a reason for hiding this comment

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

Hi Humphrey @HumphreyYang ,

Thank you for your review.
Maybe we can put this part into a separable issue and discuss with John first.

Best,
Longye

Copy link
Contributor

Choose a reason for hiding this comment

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

@HumphreyYang I am not familiar with the differences between centred and uncentred. In most econometrics books I have read it focuses on ESS, TSS etc. without specifying.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think

$$
\text{TSS} = \text{ESS} + \text{SSR}
$$

would be a nice tidy up addition.

Copy link
Author

Choose a reason for hiding this comment

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

@mmcky , I'll update this part to tidy up

\draw[important line, blue,->] (O) -- (y) node[right] {$y$};
\draw[important line,blue,thick, ->] (O) -- (Py');
Copy link
Member

Choose a reason for hiding this comment

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

I built this but it still generates the same old figure, so I updated this line to

    \draw[important line, blue,->]  (O) -- (Py') node[anchor = north west, text width=5em] {$P y'$};

following the previous lines which gives the blue arrow!

Copy link

github-actions bot commented Aug 5, 2025

@github-actions github-actions bot temporarily deployed to pull request August 5, 2025 01:22 Inactive
@jstac
Copy link
Contributor

jstac commented Aug 5, 2025

Many thanks @longye-tian for kicking this off and very close reading of the lecture.

Thanks @mmcky and @HumphreyYang for polishing! I'll leave it for you to finish up and merge.

@mmcky
Copy link
Contributor

mmcky commented Aug 6, 2025

thanks @longye-tian and @HumphreyYang.

@longye-tian when you have time would you mind updating from the latest main to fix the merge conflict?

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

thanks @longye-tian.

I think my suggestions above will fix this.

Typically the directive is reserved (for the parser) on a separate line (and sometimes a title can be specified) but contents should be on a newline within a directive.

@longye-tian
Copy link
Author

Secondly, the new plot we put in seems a bit too large compared to other ones, please see the following comparison, the first one is the one we changed, the second on is other graph

Screenshot 2025-08-07 121015 Screenshot 2025-08-07 121430

Would anyone please help me a bit on both problems?

Thank you very much.

Longye

@longye-tian
Copy link
Author

thanks @longye-tian.

I think my suggestions above will fix this.

Typically the directive is reserved (for the parser) on a separate line (and sometimes a title can be specified) but contents should be on a newline within a directive.

Thank you @mmcky . I'll try your suggestions. I'm a bit confused as all other proof env are good. Only this one doesn't work

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

thanks @longye-tian that is because the new image that has been generated is larger than the old image.

You can use :scale: as a parameter to a figure and image directives, perhaps use 75%

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

thanks @longye-tian.
I think my suggestions above will fix this.
Typically the directive is reserved (for the parser) on a separate line (and sometimes a title can be specified) but contents should be on a newline within a directive.

Thank you @mmcky . I'll try your suggestions. I'm a bit confused as all other proof env are good. Only this one doesn't work

Roger that @longye-tian -- let me take a closer look locally.

@longye-tian
Copy link
Author

Hi @mmcky, @HumphreyYang,

One other thing that I find is that in the advanced and also intermediate lecture series, every first letter of each word in the subtitles are capitalized.

I remember when working on the intro series, our format is to only capitalize the first letter right?

Shall we put up one big issue to fix this common problem? I find this is in almost all the lectures.

Best,
Longye

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

Hi @mmcky, @HumphreyYang,

One other thing that I find is that in the advanced and also intermediate lecture series, every first letter of each word in the subtitles are capitalized.

I remember when working on the intro series, our format is to only capitalize the first letter right?

Shall we put up one big issue to fix this common problem? I find this is in almost all the lectures.

Best, Longye

Thanks @longye-tian that is correct https://manual.quantecon.org/styleguide/writing.html#titles-and-headings

Can you please open an issue on https://github.com/quantecon/meta and then add checkboxes for each lecture series so we can use that as a tracker across each series?

This would be a good task for copilot.

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

@longye-tian the reason for the prf directive not working is fixed by 850cfbd

The reason I gave you wasn't causing the issue (but it is good practice). The error was because in the previous prf there was a nested math directive using the ticks so the enclosing prf directive should use 4 x ' to create a closure around that nested directive.

Does that make sense? The parser doesn't know how to figure out closures automatically.

@longye-tian
Copy link
Author

Hi @mmcky, @HumphreyYang,
One other thing that I find is that in the advanced and also intermediate lecture series, every first letter of each word in the subtitles are capitalized.
I remember when working on the intro series, our format is to only capitalize the first letter right?
Shall we put up one big issue to fix this common problem? I find this is in almost all the lectures.
Best, Longye

Thanks @longye-tian that is correct https://manual.quantecon.org/styleguide/writing.html#titles-and-headings

Can you please open an issue on https://github.com/quantecon/meta and then add checkboxes for each lecture series so we can use that as a tracker across each series?

This would be a good task for copilot.

Copy that @mmcky . I'll list the lecture names in check box for the one has this common problem.

I'm not sure how to use copilot to fix it automatically though.

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

I'm not sure how to use copilot to fix it automatically though.

@longye-tian this is a topic we will discuss on Monday. But more than happy to link up with you today or tomorrow as well if you have time.

@longye-tian
Copy link
Author

@longye-tian the reason for the prf directive not working is fixed by 850cfbd

The reason I gave you wasn't causing the issue (but it is good practice). The error was because in the previous prf there was a nested math directive using the ticks so the enclosing prf directive should use 4 x ' to create a closure around that nested directive.

Does that make sense? The parser doesn't know how to figure out closures automatically.

Thank you @mmcky . I see in the previous theorem, there is a {math} inside, so to create a bigger env for theorem, we need {prf:theorem} right?

@longye-tian
Copy link
Author

I'm not sure how to use copilot to fix it automatically though.

@longye-tian this is a topic we will discuss on Monday. But more than happy to link up with you today or tomorrow as well if you have time.

Thank you @mmcky , I'm free this afternoon. Shall we have a zoom meeting at your convenience?

@mmcky
Copy link
Contributor

mmcky commented Aug 7, 2025

I see in the previous theorem, there is a {math} inside, so to create a bigger env for theorem, we need {prf:theorem} right?

The only change I made in 850cfbd is to use 4 ticks rather than three. That leaves the math directive to use 3 ticks. This enables the parser to match the appropriate start and end to each directive. Does that answer your question?

@longye-tian
Copy link
Author

I see in the previous theorem, there is a {math} inside, so to create a bigger env for theorem, we need {prf:theorem} right?

The only change I made in 850cfbd is to use 4 ticks rather than three. That leaves the math directive to use 3 ticks. This enables the parser to match the appropriate start and end to each directive. Does that answer your question?

I think I got it. I'll be careful next time when I see something nested.

@github-actions github-actions bot temporarily deployed to pull request August 7, 2025 02:47 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 7, 2025 05:23 Inactive
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.

4 participants