Skip to content
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

Dynamic Interest Rate #1221

Merged
merged 6 commits into from
Feb 6, 2023
Merged

Conversation

alanlujan91
Copy link
Member

@alanlujan91 alanlujan91 commented Feb 2, 2023

This should close one of #1106 or #1132 or both. Should also address sbenthall/SHARKFin#186 (comment).

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

Can @wdu9 and @dedwar65 also review this?

With apologies to @dedwar65, this might affect #1219. (Going back to agent.Rfree without index in notebook).

The logic is as follows:
If Rfree is an int, float: it is time invariant.
If Rfree is a list of length 1: it is time invariant and recast as a float.
If Rfree is a list of length n>1: it is time varying and should be same length as T_cycle.
If Rfree is an array: it is time invariant (for Markov types)

Everywhere else where we want to use Rfree (as in calculations) we should first check the type then use appropriately (indexed if time varying).

@alanlujan91 alanlujan91 marked this pull request as draft February 2, 2023 01:35
@alanlujan91 alanlujan91 marked this pull request as ready for review February 2, 2023 01:36
@alanlujan91 alanlujan91 changed the title Dynamic Interest Rate [WIP] Dynamic Interest Rate Feb 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2023

Codecov Report

Base: 73.32% // Head: 73.30% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (8cf89b3) compared to base (69e0aa3).
Patch coverage: 85.71% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1221      +/-   ##
==========================================
- Coverage   73.32%   73.30%   -0.03%     
==========================================
  Files          74       74              
  Lines       12122    12120       -2     
==========================================
- Hits         8889     8885       -4     
- Misses       3233     3235       +2     
Impacted Files Coverage Δ
...mptionSaving/tests/test_ConsPortfolioFrameModel.py 100.00% <ø> (ø)
HARK/ConsumptionSaving/ConsIndShockModel.py 87.02% <84.61%> (-0.14%) ⬇️
HARK/ConsumptionSaving/ConsRiskyAssetModel.py 40.14% <100.00%> (-0.89%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@llorracc
Copy link
Collaborator

llorracc commented Feb 2, 2023

This seems like a sound temporary fix.

But this problem illustrates why my strongly held view is that the default way of building models should be to define a terminal period, then define the problem for $T-1$, then define the solution for $T-2$, and so on. Part of the "defining" would be to specify the value for $R$ that is period-specific.

My way of doing things would get rid of a lot of extraneous baggage (like this).

Wish we'd done it that way from the start. (My fault that we didn't).

@dedwar65
Copy link
Contributor

dedwar65 commented Feb 3, 2023 via email

@alanlujan91
Copy link
Member Author

@dedwar65 if this gets merged before yours, then the process should be to merge master back to your PR, then address any conflicts that come from that. Trying to preempt changes before they are on master might cause more issues. We can work on it together once it's needed.

@alanlujan91 alanlujan91 added the Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged label Feb 3, 2023
@alanlujan91 alanlujan91 changed the title [WIP] Dynamic Interest Rate Dynamic Interest Rate Feb 6, 2023
@llorracc
Copy link
Collaborator

llorracc commented Feb 6, 2023

@alanlujan91, this is not marked "Ready-To-Merge" but my sense is that you think it is?

I guess we still don't have a workflow to test a PR against DemARKs; but if I merge it, can you trigger a retest of the DemARKs and reverse my merge if problems crop up?

@alanlujan91
Copy link
Member Author

This is tagged as Ready to Merge. Not sure why you can't see it. But yes I can revert if needed.

@llorracc
Copy link
Collaborator

llorracc commented Feb 6, 2023

Oh, I see the Ready-To-Merge label now. Didn't know where to look.

Merging ...

@alanlujan91 alanlujan91 added this to the 0.13.0 milestone Feb 6, 2023
@alanlujan91 alanlujan91 merged commit 9f854cd into econ-ark:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged
Projects
None yet
4 participants