-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
Transition Matrix Methods #1155
Conversation
Codecov ReportBase: 73.62% // Head: 72.83% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1155 +/- ##
==========================================
- Coverage 73.62% 72.83% -0.79%
==========================================
Files 72 72
Lines 11509 11735 +226
==========================================
+ Hits 8473 8547 +74
- Misses 3036 3188 +152
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. |
Will,
If you think this is ready to merge, could you add a tag to that effect to
it (we have a predefined tag for that).
…On Thu, Jul 21, 2022 at 3:10 AM wdu9 ***@***.***> wrote:
Please ensure your pull request adheres to the following guidelines:
- [x ] Tests for new functionality/models or Tests to reproduce the
bug-fix in code.
- [x ] Updated documentation of features that add new functionality.
- [x ] Update CHANGELOG.md with major/minor changes.
------------------------------
You can view, comment on, or merge this pull request online at:
#1155
Commit Summary
- eb2d369
<eb2d369>
Transition Matrix Methods
- 6d55939
<6d55939>
Merge branch 'econ-ark:master' into Transition_Matrix
- 01f1dad
<01f1dad>
transition Matrix methods
- 6dd504c
<6dd504c>
Merge branch 'Transition_Matrix' of https://github.com/wdu9/HARK into
Transition_Matrix
File Changes
(5 files <https://github.com/econ-ark/HARK/pull/1155/files>)
- *M* Documentation/CHANGELOG.md
<https://github.com/econ-ark/HARK/pull/1155/files#diff-642febef01c09084c712d527a5b0a1c04eb1fb108202c2a1445947f31db48f07>
(2)
- *M* HARK/ConsumptionSaving/ConsIndShockModel.py
<https://github.com/econ-ark/HARK/pull/1155/files#diff-7f09d28a3d4136ae35d8835cd1352f060da6e0114fe617c34374c8b57d41a57e>
(300)
- *M* HARK/ConsumptionSaving/tests/test_IndShockConsumerType.py
<https://github.com/econ-ark/HARK/pull/1155/files#diff-46adcdf1fe37ad7df97b9cb94fdba2e287e910387c68b9b1158c146eb5ccba85>
(47)
- *M* HARK/utilities.py
<https://github.com/econ-ark/HARK/pull/1155/files#diff-30bbf6349341e5b97b6a40790de996e53763b5e37e896cb12ab6254ee024e439>
(150)
- *A*
examples/ConsIndShockModel/IndShockConsumerType_Transition_Matrix_Example.ipynb
<https://github.com/econ-ark/HARK/pull/1155/files#diff-ee32ef5bc9227ad4f730a96c49d67410b11809c46fa69f69cb5493e29c7a092a>
(1075)
Patch Links:
- https://github.com/econ-ark/HARK/pull/1155.patch
- https://github.com/econ-ark/HARK/pull/1155.diff
—
Reply to this email directly, view it on GitHub
<#1155>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCK7Y7S67URBPGYXE7ENTVVDZUZANCNFSM54GQDPOQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
- Chris Carroll
|
This is ready to be merged. I think we discussed this before but I don't think I am able to add tags. |
Happy to review this |
#Dist_pGrid is taken to cover most of the ergodic distribution | ||
p_variance = self.PermShkStd[0]**2 #set variance of permanent income shocks | ||
max_p = max_p_fac*(p_variance/(1-self.LivPrb[0]))**0.5 #Maximum Permanent income value | ||
one_sided_grid = make_grid_exp_mult(1.05+1e-3, np.exp(max_p), num_points, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min_p should maybe be a parameter to this model instead of hardcoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, good catch.
For calc_ergodic_dist: (based on numerical summer school lecture)
Should be faster then searching for the eigenvalues with sp.linalg.eigs |
jump_to_grid, | ||
jump_to_grid_fast, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More descriptive names would be useful for extensions/alternative uses. In the past, we've used _fast
to refer to numba-fied methods that do the same as their reference methods, but here, both are using numba.jit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok got it.
for i in range(m_density): | ||
axtra_shifted = np.delete(aXtra_Grid,-1) | ||
axtra_shifted = np.insert(axtra_shifted, 0,1.00000000e-04) | ||
dist_betw_pts = aXtra_Grid - axtra_shifted | ||
dist_betw_pts_half = dist_betw_pts/2 | ||
new_A_grid = axtra_shifted + dist_betw_pts_half | ||
aXtra_Grid = np.concatenate((aXtra_Grid,new_A_grid)) | ||
aXtra_Grid = np.sort(aXtra_Grid) | ||
self.dist_mGrid = aXtra_Grid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this "thickening" the grid of the distribution of cash-on-hand? what is particular about this construction that is different from make_grid_exp_mult
? If this is doing something that can't be accomplished by make_grid_exp_mult
maybe it should be its own utility method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does thicken if that means to increase the density. Chris had asked me to do this a while back but I'm not sure whethe rmake_grid_exp_mult can thicken, I feel as though it just concentrates points near the bottom end.
elif self.cycles > 1: | ||
raise Exception('define_distribution_grid requires cycles = 0 or cycles = 1') | ||
|
||
elif self.T_cycle != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this condition is (cycles == 1 and T_cycle !=0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is T_cycle < 1
ever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this condition is meant as (cycles == 1 and T_cycle !=0).
T_cycle is never less than 1 I believe
else: | ||
m_points = num_pointsM | ||
|
||
if self.cycles == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be
if (self.cycles == 0) or (self.cycles == 1 and self.T_cycle !=0):
to avoid code repetition I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The T_cycle !=0 problem is exclusive of the cycles=0 problem so it is not unfortunately.
LivPrb = self.LivPrb[0] # Update probability of staying alive | ||
|
||
#New borns have this distribution (assumes start with no assets and permanent income=1) | ||
NewBornDist = jump_to_grid(tran_shks,np.ones_like(tran_shks),shk_prbs,dist_mGrid,dist_pGrid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a probability distribution? Would it be useful to use our DiscreteDistribution
constructor here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a function that takes in a grid, a set of values that may lie between the gridpoints, and finally the associated probabilities of each the values and returns the probability mass of each gridpoint. I don't think itd be useful to use DiscreteDistribution here
elif self.cycles > 1: | ||
raise Exception('calc_transition_matrix requires cycles = 0 or cycles = 1') | ||
|
||
elif self.T_cycle!= 0 : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a model that has cycles==1 and T_cycle==0? I'm thinking this should never happen, and if it does, it's probably a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, that is why I was so specific on the conditional cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few in-line comments. Overall looks great!
Comments about new tools or using DiscreteDistribution
can probably be addressed later.
e64c32d
to
ec3a7bf
Compare
@wdu9 , Glad it is now passing tests. Will ask @alanlujan91 to make a final review before merging. (Thanks to @AMonninger for earlier comments; maybe he could take a final look with an eye to how it might work for his durables problem; but @alanlujan91 should be the decider about merging). @wdu, one thing I'd ask you to add to the chain is some insight about why it was failing before and is now working. What did you change? If it's another instance of our tests just defaulting to a degree of precision that doesn't make sense, that would be a useful insight. If it's something else, that would also be useful to know. |
I had this in the code however sometimes it would fail for unknown reasons. It was indeed twice as fast at very large grid dimensions. |
I reduced the tolerance of the self.assertalmostequal function to resolve the original problem where the self assertion was off by a difference of 10e-6. |
Hi Will, some additional comments:
|
3c2d1b7
to
e6c936b
Compare
Please ensure your pull request adheres to the following guidelines: