Newborns' death probabilities. #981
Conversation
|
The problem you're describing shouldn't happen. The newborns are put into
the set of agents *after* the death event happens. See
AgentType.get_mortality(), which runs sim_death() and *then*
sim_birth(who_dies). The first opportunity an agent has to die is at the
very beginning of their second period of life, when t_age=1 and t_cycle=1
(in a lifecycle model). Thus it will correctly pull from LivPrb[0] for
these agents. That's what the comment, "Time has already advanced, so look
back one" means in ConsIndShockModel's sim_death() code.
If it *does* happen, we need to fix it.
…On Mon, Mar 8, 2021 at 10:30 PM Mateo Velásquez-Giraldo < ***@***.***> wrote:
The current implementation of sim_death finds death probabilities as DiePrb
= DiePrb_by_t_cycle[self.t_cycle - 1].
For newborns, t_cycle==0 so this becomes DiePrb_by_t_cycle[-1]. Thus,
newborns are getting the last survival probability, which in life-cycle
calibrations is usually low. Therefore, many die immediately after being
born.
Therefore, I'd say this bug results in the unnecessary deaths of many
newborns. If that is not worrying enough, I think it may cause many
simulation observations to be wasted, as they are spent on instantly-dying
newborns.
I am not the most familiar with t_cycle vs t_age. Could someone more
familiar review?
- 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:
#981
Commit Summary
- Set newborn's death probabilities to 0
File Changes
- *M* HARK/ConsumptionSaving/ConsIndShockModel.py
<https://github.com/econ-ark/HARK/pull/981/files#diff-7f09d28a3d4136ae35d8835cd1352f060da6e0114fe617c34374c8b57d41a57e>
(5)
Patch Links:
- https://github.com/econ-ark/HARK/pull/981.patch
- https://github.com/econ-ark/HARK/pull/981.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#981>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFMSRPOPEF6LT3RAJ3DTCWI5XANCNFSM4Y2U3GZA>
.
|
|
Thanks! I was running into the issue yesterday. I was pretty convinced that the problem was happening, at least in the very first period of simulations. Maybe the issue is that I'll double check soon and get more evidence. |
|
Yeah, it's possible that it happens in the very first simulated period, but
that's harmless-- It just means one batch of newborns is immediately
replaced by another batch before they ever do anything. It's still
random draws of newborns.
…On Tue, Mar 9, 2021 at 10:13 AM Mateo Velásquez-Giraldo < ***@***.***> wrote:
Thanks!
I was running into the issue yesterday. I was pretty convinced that the
problem was happening, at least in the very first period of simulations.
Maybe the issue is that initialize_sim() spawns newborns and then sim_one_period
-> get_mortality kills them. I'll double check soon and get more evidence.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#981 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFPTCNW5HBAOGM6EFSTTCYUIPANCNFSM4Y2U3GZA>
.
|
|
Might be related to this issue |
|
I've double checked. This is an issue that happens only once. As you said, it is harmless: a set of newborns is replaced with another. Then the question is whether you want it to happen or not. The advantages of the proposed fix are:
The drawback:
If you'd rather not incorporate it, this can be closed. |
|
I would recommend using a performance testing tool to resolve these questions about implementation speed. https://docs.python.org/3/library/profile.html As a general point, using a profiler to test for performance bottlenecks would be a great thing to do at this stage. Made #982 for this |
|
My suspicion is that it will be an extremely small fraction of the runtime
of a structural estimation's objective function, on the order of about
1/1,000,000 of the execution time.
…On Wed, Mar 10, 2021 at 1:21 PM Sebastian Benthall ***@***.***> wrote:
I would recommend using a performance testing tool to resolve these
questions about implementation speed.
https://docs.python.org/3/library/profile.html
As a general point, using a profiler to test for performance bottlenecks
would be a great thing to do at this stage. Made #982
<#982> for this
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#981 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFOWTX3SMMQWBFZHUTDTC6TAJANCNFSM4Y2U3GZA>
.
|
|
For things like this, the best resolution is one that prevents any future wasted HUMAN time, so (a) whatever is less confusing and (b) add a comment so any future @Mv77 will not occupy more than 30 sec grokking it |
|
Tests are not passing. |
|
Could you revise the PR to just involve adding a comment or two to the existing code which, if such comment had existed 9 months ago would have prevented you from confusion? (The comment should include a link to this (revised) PR. |
Codecov Report
@@ Coverage Diff @@
## master #981 +/- ##
==========================================
+ Coverage 71.67% 71.76% +0.09%
==========================================
Files 63 63
Lines 9197 9256 +59
==========================================
+ Hits 6592 6643 +51
- Misses 2605 2613 +8
Continue to review full report at Codecov.
|
This is done in the last few commits. This PR is ready to merge now. Sorry for throwing everyone into this rabbit hole. |
The current implementation of
sim_deathfinds death probabilities asDiePrb = DiePrb_by_t_cycle[self.t_cycle - 1].For newborns,
t_cycle==0so this becomesDiePrb_by_t_cycle[-1]. Thus, newborns are getting the last survival probability, which in life-cycle calibrations is usually low. Therefore, many die immediately after being born.Therefore, I'd say this bug results in the unnecessary deaths of many newborns. If that is not worrying enough, I think it may cause many simulation observations to be wasted, as they are spent on instantly-dying newborns.
I am not the most familiar with
t_cyclevst_age. Could someone more familiar review?