Add reshuffling method [WIP] #1244
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1244 +/- ##
==========================================
- Coverage 73.31% 73.05% -0.26%
==========================================
Files 76 75 -1
Lines 12548 12576 +28
==========================================
- Hits 9199 9188 -11
- Misses 3349 3388 +39
... and 16 files with indirect coverage changes 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 in Codecov by Sentry. |
|
Yay! Very exciting to see this!
On Sun, Mar 19, 2023 at 7:59 PM wdu9 ***@***.***> wrote:
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.
------------------------------
You can view, comment on, or merge this pull request online at:
#1244
Commit Summary
- 36d7cbf
<36d7cbf>
add reshuffling method
File Changes
(1 file <https://github.com/econ-ark/HARK/pull/1244/files>)
- *M* HARK/ConsumptionSaving/ConsIndShockModel.py
<https://github.com/econ-ark/HARK/pull/1244/files#diff-7f09d28a3d4136ae35d8835cd1352f060da6e0114fe617c34374c8b57d41a57e>
(162)
Patch Links:
- https://github.com/econ-ark/HARK/pull/1244.patch
- https://github.com/econ-ark/HARK/pull/1244.diff
—
Reply to this email directly, view it on GitHub
<#1244>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCK73WWLCO2EYRVSVXRX3W46MWJANCNFSM6AAAAAAWANH23M>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
Sent from Gmail Mobile
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1244 +/- ##
==========================================
+ Coverage 72.64% 72.71% +0.07%
==========================================
Files 78 78
Lines 13028 13106 +78
==========================================
+ Hits 9464 9530 +66
- Misses 3564 3576 +12
☔ View full report in Codecov by Sentry. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
||
| if self.perf_reshuffle == True:# when true, permanent and transitory shocks are evenly distributed across both newborns and non newborns. | ||
| Min_AgentCount = check_and_convert_to_int(lcm/(1-self.LivPrb[0])) # total number of agents | ||
| if (self.AgentCount/Min_AgentCount).is_integer() == False: # check if Agentcount is appropriate to implement perfect reshuffling |
There was a problem hiding this comment.
There's no need to test if a boolean == False in an if statement.
There was a problem hiding this comment.
What I mean is... it's better style to use a 'not' here (i.e. if not (...).is_integer():
https://www.w3schools.com/python/ref_keyword_not.asp
... or to do if (expression that evaluates to true) ... else: (do the thing on the condition that it's false)
This is just a style nitpick.
|
Hi @wdu9 I'm looking over this PR and trying to understand the parts of it. I'm seeing three things:
Am I reading this right? Can you tell me if anything else is happening that I'm missing? Any explanation of this could go into the docstrings to improve the API documentation, which is part of the checklist for merging PRs. |
|
On Thu, Aug 10, 2023 at 5:03 PM Sebastian Benthall ***@***.*** ***@***.***> wrote:
Hi @wdu9 <https://github.com/wdu9>
I'm looking over this PR and trying to understand the parts of it. I'm
seeing three things:
- passing self.reshuffle to the exact_match argument when shocks are
drawn from a distribution
- A test to make sure that the the AgentCount is an exact multiple of
the MinAgentCount (which I guess is the number of equiprobably discretized
parts of the distribution)
My strong preference is that, when using the reshuffling method, the user
is required to provide an integer-valued input variable named replicants or
multiples or something; AgentCount should be generated as AgentCount =
MinAgentCount * multiples
- A third thing that I'm not sure I understand, having to with
unemployment draws?
Unemployment is effectively one more possible value of the transitory
shock; so, if the unemployment rate is 5 percent, you’d need to have
MinAgentCount_unemp
= 20 agents in order to have exactly one person unemployed every period
https://github.com/econ-ark/HARK/pull/1244/files#diff-7f09d28a3d4136ae35d8835cd1352f060da6e0114fe617c34374c8b57d41a57eR2273-R2301
Am I reading this right? Can you tell me if anything else is happening
that I'm missing?
Any explanation of this could go into the docstrings to improve the API
documentation, which is part of the checklist for merging PRs.
—
Reply to this email directly, view it on GitHub
<#1244 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKCK75RMAY7ZVGJ24OT4QTXUVEDTANCNFSM6AAAAAAWANH23M>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
- Chris Carroll
|
|
Hi @sbenthall , Yes you are exactly correct. Much of the code is written to check whether the number of agents is the correct multiple needed to implement the reshuffling method. If the number of agents is appropriate then reshuffle calls exact_match which does most of the heavy lifting by allowing draws to be drawn "perfectly". The other method that was altered was sim_death to ensure that the number of agents that die each period is exactly (1-LivPrb). I plan to make the code clearer as the current code is crude in how it determines if AgentCount is the correct multiple. It does work however. |
Please ensure your pull request adheres to the following guidelines: