Skip to content

Modified this apparently on flight home from CEF Singapore#1505

Open
llorracc wants to merge 2 commits intomainfrom
Qatar-Airways-M2
Open

Modified this apparently on flight home from CEF Singapore#1505
llorracc wants to merge 2 commits intomainfrom
Qatar-Airways-M2

Conversation

@llorracc
Copy link
Collaborator

@wdu9, @akshayshanker , could you both review?

@wdu9, there are some html comments embedded for you. Search CDC.
Please ensure your pull request adheres to the following guidelines:

  • Tests for new functionality/models or Tests to reproduce the bug-fix in code.
  • Updated documentation of features that add new functionality.
  • Update CHANGELOG.md with major/minor changes.

@llorracc llorracc requested a review from wdu9 October 26, 2024 22:07
@akshayshanker
Copy link
Contributor

@wdu9 do you want to make your particular edits in response to the @CDC2WXD comments in SSJ_explanation.ipynb, and then I can review?

@llorracc, I did a pretty extensive tidy up of KS-HARK-Presentation.ipynb after you had done these edits. I suggest keeping the KS-HARK-Presentation.ipynb I edited. Let's quickly discuss in person next Zoom.

@akshayshanker
Copy link
Contributor

Scratch that --- I will address the comments including the ones to @wdu9.

@mnwhite
Copy link
Contributor

mnwhite commented May 21, 2025

I think this PR is going to be harder to resolve. It looks like it's from even earlier than the prior one, and the files have been changed substantially since then. The "new" file in this PR might actually be an old version of a different file, under an old name. I guess Akshay will tackle this in a fresh branch?

Funny enough, CDC's primary comment from June 2024 was exactly what I wrote in my review on the other PR (unaware Chris wrote it here): the model needs to be explained in math and/or words.

@llorracc
Copy link
Collaborator Author

Thanks @mnwhite

Do you have a complete inventory of the HANK related notebooks and material in HARK? If so, is that inventory described somewhere that would be easy to find for someone looking for precisely this material? If not, I think it would be very useful to have such an inventory.

@mnwhite
Copy link
Contributor

mnwhite commented May 21, 2025

The SSJ and HANK-adjacent notebooks were moved from /examples/SSJ-example/ to /examples/ConsNewKeynesianModel/ in the master branch. There are currently five notebooks there:

  • HANKFiscal_example.ipynb
  • Jacobian_example.ipynb
  • KS-HARK-presentation.ipynb
  • SSJ_explanation.ipynb
  • Transition_Matrix_Example.ipynb

I think KrusellSmith_solved_by_HANK.ipynb in this branch is a very old/preliminary version of what is now HANKFiscal_example.ipynb, but don't hold me to that.

mnwhite added a commit that referenced this pull request May 28, 2025
This is a recreation of the substantive changes from #1505 . Requested / suggested changes that CDC left as comments were not made.
@mnwhite
Copy link
Contributor

mnwhite commented May 28, 2025

Substantive changes have been incorporated via #1568

@mnwhite mnwhite closed this May 28, 2025
@github-project-automation github-project-automation bot moved this from In progress to Done in Issues & PRs May 28, 2025
@mnwhite mnwhite reopened this May 28, 2025
@github-project-automation github-project-automation bot moved this from Done to In progress in Issues & PRs May 28, 2025
@mnwhite mnwhite removed Priority: High Version: 0.16 for now-released v0.16.0 labels May 28, 2025
@mnwhite
Copy link
Contributor

mnwhite commented May 28, 2025

Re-opened because Akshay wants it left as to be completed.

@mnwhite
Copy link
Contributor

mnwhite commented Jun 4, 2025

Can this PR be archived now in some way? My understanding is that the remaining work on this is to address some comments from CDC that require new writing. Could those comments be moved to an issue so that we can close out a PR that will never be merged because it's been superceded?

@llorracc
Copy link
Collaborator Author

llorracc commented Jun 5, 2025

@mnwhite, I'm OK if you want to turn them into an issue. You should be able to compare he commit before this one to this commit to see what I changed/edited.

@mnwhite mnwhite moved this from In progress to Stale in Issues & PRs Jan 3, 2026
@alanlujan91 alanlujan91 requested a review from Copilot January 28, 2026 19:15
Copy link
Contributor

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 refines the SSJ explanation notebook to give a more pedagogical introduction to the sequence-space Jacobian method and its application to a (simplified) Krusell–Smith model, and adds a reference to a companion HANK–SSJ notebook.

Changes:

  • Rewrote the introductory description of the SSJ method, including motivation relative to state-space approaches and the treatment of MIT shocks and heterogeneity.
  • Clarified several parts of the model exposition (e.g., definition of market resources, TFP dynamics, sequence-space system formulation, and steady-state linearization narrative).
  • Added a short section linking to a companion “Krusell-Smith-Solved-By-HANK-and-SSJ” notebook and updated notebook metadata and closing explanation text.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"# The Sequence Space Jacobian (SSJ) method\n",
"\n",
"\n",
"<!-- CDC2WXD: Put in a link to the paper where SSJ is defined -→\n"
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

This HTML comment appears to be an internal TODO for a specific reviewer ("CDC2WXD") and is also syntactically invalid because it is never closed with a terminating -->, which can break Markdown/HTML rendering by commenting out subsequent content. Please either remove this reviewer note or convert it to regular Markdown text, and if you still need an HTML comment here, ensure it is properly closed.

Copilot uses AI. Check for mistakes.
"metadata": {},
"source": [
"## Krusell Smith Model in Sequence Space"
"## Krusell Smithy Model in Sequence Space\n",
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The section header uses "Krusell Smithy Model" instead of the standard name "Krusell–Smith model", which is likely a typo and could confuse readers who search for this model by name. Please correct the spelling of the model name in this heading to match the canonical literature.

Suggested change
"## Krusell Smithy Model in Sequence Space\n",
"## Krusell–Smith model in Sequence Space\n",

Copilot uses AI. Check for mistakes.
"## Krusell Smithy Model in Sequence Space\n",
"\n",
"Here we solve a slightly simplified version of the Krusell-Smith model (the simplifications are to clarify the exposition).\n",
"<!-- CDC2WXD: Explain what you are doing here. It is not the classic KS model, it's a simplified one like the baseline version in our KrusellSmith notebook. Explain and make links -→"
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

This line is another internal reviewer note in an unterminated HTML comment (missing the closing -->), which risks commenting out following content in some renderers and should not ship in an example notebook. Please remove or rewrite this as regular Markdown and, if you still prefer to keep it as a comment, ensure the HTML comment is syntactically closed.

Copilot uses AI. Check for mistakes.
"# The model as a system of difference equations in sequence space"
"# Model as defined by diff eqns in sequence space\n",
"\n",
"<!-- CDC2WXD: You need to have some citation(s) here! -→"
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

This is a third internal TODO-style note in an unterminated HTML comment, which is both invalid HTML and likely not intended for end users of the notebook. Please either remove or convert this note into normal Markdown prose, and if you keep it as an HTML comment, make sure to close it with --> so it does not interfere with rendering.

Copilot uses AI. Check for mistakes.
"source": [
"## Solving Using the HANK-SSJ Link\n",
"\n",
"A companion notebook, `Krusell-Smith-Solved-By-HANK-and-SSJ` presents the solution to the model described above using the HARK toolkit in combination with the SSJ toolkit."
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

This paragraph refers to a companion notebook Krusell-Smith-Solved-By-HANK-and-SSJ, but there is no file with this name in the repository (searching for this string only finds this reference). Either add the referenced notebook to the repo or update the text to point to an existing resource so users are not directed to a non-existent example.

Suggested change
"A companion notebook, `Krusell-Smith-Solved-By-HANK-and-SSJ` presents the solution to the model described above using the HARK toolkit in combination with the SSJ toolkit."
"A separate companion notebook demonstrates how to solve the model described above using the HARK toolkit in combination with the SSJ toolkit. (This example notebook is not included in this repository.)"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Stale

Development

Successfully merging this pull request may close these issues.

4 participants