Skip to content

[aiyagari] MAINT: transfer np.sum(a * b) to a @ b #467

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 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mmcky
Copy link
Contributor

@mmcky mmcky commented Jun 19, 2025

This PR fixes part of #463

  • apply fix and compare to original code
  • remove original code and comparison code

@mmcky mmcky changed the title MAINT: transfer np.sum(a * b) to a @ b [aiyagari] MAINT: transfer np.sum(a * b) to a @ b Jun 19, 2025
Copy link

github-actions bot commented Jun 19, 2025

@github-actions github-actions bot temporarily deployed to pull request June 19, 2025 03:10 Inactive
@mmcky mmcky requested review from HengchengZhang, Copilot and HumphreyYang and removed request for HengchengZhang June 19, 2025 04:02
Copy link

@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 updates the Aiyagari lecture notebook by synchronizing Jupytext metadata, standardizing code-cell directives, and refactoring the capital‐supply calculation to use the @ operator instead of np.sum.

  • Add format_version and jupytext_version to notebook metadata and adjust kernel display name.
  • Change all {code-cell} directives to use ipython3 and the :tags: syntax.
  • Introduce prices_to_capital_stock and replace np.sum(a * b) with asset_probs @ am.a_vals, retaining the original results for comparison.
Comments suppressed due to low confidence (2)

lectures/aiyagari.md:472

  • [nitpick] The parameter name am is ambiguous; consider renaming it to something like household or hh to clarify that it’s an instance of the Household class.
def prices_to_capital_stock(am, r):

lectures/aiyagari.md:464

  • [nitpick] Function name rd is not very descriptive; consider renaming it to something like interest_rate_from_capital or capital_demand_rate for clarity.
def rd(K):

Comment on lines +501 to +502
am_ddp = DiscreteDP(am.R, am.Q, am.β)

Copy link
Preview

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

The variable am_ddp is assigned but never used; it can be removed to reduce clutter.

Suggested change
am_ddp = DiscreteDP(am.R, am.Q, am.β)

Copilot uses AI. Check for mistakes.

@mmcky
Copy link
Contributor Author

mmcky commented Jun 19, 2025

@HumphreyYang this is an easy one, but would you mind reading through #463 (comment) and letting me know if you approve of this change to:

  • verify change is correct
  • check the assert between original code and new code

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.

Hi @mmcky, this looks great to me.

I think we can go ahead with a global switch, as I don't expect this change to cause much trouble.

After that, we can review all the updated lectures to ensure they’re consistent with the previous results!

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.

2 participants