Some RNG fixes #757
Some RNG fixes #757
Conversation
Malfunctioning Fagereng DemARK revealed that simulation RNG was not being properly reset by the AgentType.resetRNG() method. This commit adds an extended resetRNG() method for IndShockConsumerType, which fixes the problem for that class and some of its immediate successors. Other AgentType subclasses will also need resetRNG() methods like this to make sure their simulation RNG gets reset fully.
Tested with examples file, which I also cleaned very slightly (process_time to time, removed reference to timeFwd). This will break more tests, of course.
Cleaned example file slightly; it had some references to old distribution format. Also adjusted how preference shocks are drawn to be more parsimonious with new distribution format.
Also made small fixes to example file to remove references to old distribution format. And changed how simulation draws work to simplify the code and accelerate (5x improvement).
The Markov versions of ConsAggShock and ConsRepAgent were breaking because their IncomeDstns are Markov-state dependent. These types now call MarkovConsumerType's resetRNG method rather than IndShockConsumerType's. Also twiddled the targets for some of these tests because they depend on RNG behavior in IndShockConsumerType, which changed earlier in this branch.
Some autoformatter made this solver nearly unreadable. Adjusted the position of comments to restore it.
| if hasattr(self, 'IncomeDstn'): | ||
| T = len(self.IncomeDstn) | ||
| for t in range(T): | ||
| self.IncomeDstn[t].reset() |
There was a problem hiding this comment.
possible rewrite:
for distribution in self.IncomeDstn:
distribution.reset()
| ''' | ||
| PerfForesightConsumerType.resetRNG(self) | ||
|
|
||
| # Reset IncomeDstn if it exists (it might not because resetRNG is called at init) |
There was a problem hiding this comment.
I wonder if this could be written in a more general way once rather than having more complexity added via inheritance.
This could be a time to think through how exogenous shocks get stored on the Agent object.
dolo, for example, puts all these into a collection names exogenous
| ''' | ||
| PersistentShockConsumerType.resetRNG(self) | ||
|
|
||
| # Reset MedShkDstn if it exists (it might not because resetRNG is called at init) |
There was a problem hiding this comment.
Again: if all the exogenous shock distributions were contained in a single collector, then this method could be written more efficiently by looping over that collection.
|
Great progress. I have some notes on this which I don't think should block the PR but which I think would, ideally, be addressed or at least marked for improvement:
I just want to confirm that the changes in this PR are consistent with
I wonder if now is the time to do this kind of refactoring, or if it should be slated for a different issue, given the urgency of this work. |
|
Yes, I have strong opinions about whitespace, and I don't know what black
says.
We can have an attribute like `exogenous` or `distributions` if you want,
but I think making the code work as intended is important. However, your
code snippet won't work, as sometimes the elements of `IncomeDstn` are in
nested lists. See resetRNG() for MarkovConsumerType.
…On Tue, Jul 14, 2020 at 10:52 AM Sebastian Benthall < ***@***.***> wrote:
Great progress.
I have some notes on this which I don't think should block the PR but
which I think would, ideally, be addressed or at least marked for
improvement:
1. It looks like some of the changes made in this PR are purely
formatting/whitespace issues, like when there are line breaks. I understand
that there are often strong aesthetic feelings on this point. I also feel
that objectively, these differences are trivial. I also believe that
black, the code formatting tool that is now a pre-commit hook, is very
opinionated about this, and that @MridulS <https://github.com/MridulS>
merged a PR some time ago with the black changes made to the
ConsumerSaving classes.
I just want to confirm that the changes in this PR are consistent with
black, and not a reversion to a pre-black standard.
1. The resetRNG is in this PR inherited and modified by each subclass
according to the name of its exogenous shocks. It would simplify the code
and inheritance structure, as well as bring the software closer in line
with Dolo, if the exogenous shocks were stored in some collection, such as
a dictionary stored at self.exogenous, instead of added as attributes
to the base Agent object. This would enable the method to be written
generically like so:
for distribution in self.exogenous:
self.exogenous[distribution].reset()
I wonder if now is the time to do this kind of refactoring, or if it
should be slated for a different issue, given the urgency of this work.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#757 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFL4NRIE6HQOXFQAU2LR3RWLPANCNFSM4OXNUIDQ>
.
|
|
Good point about the code snippet not working. I suppose I'll flag this as something to refactor later, unless you have an objection to this in principle. As for the whitespace issues, while we have not passed an EEP about it, @MridulS and I at least are in agreement that following Since there is now the pre-commit hook installed ( see #703 ) I think that creates de facto pressure to standardize this way. Running the I'm not certain but I believe some of these reformattings are ones you may have reverted in this PR. It would be extremely silly if some of us used a reformatting tool as part of our contributions, and some of us reverted them back to a different format whenever they worked on the code. |
|
In this case, it's not a multivariate distribution, but instead one that
varies by age (t) and by a discrete state (n). So there's no single
representation of the distribution within a period.
…On Tue, Jul 14, 2020 at 12:39 PM Sebastian Benthall < ***@***.***> wrote:
Good point about the code snippet not working.
Ideally, I'd like to see IncomeDstn implemented in such a way that it was
never a nested list, but always a Distribution, even if it was a complex,
multivariate distribution.
I'd see that as another step in this proposed refactoring.
I suppose I'll flag this as something to refactor later, unless you have
an objection to this in principle.
As for the whitespace issues, while we have not passed an EEP about it,
@MridulS <https://github.com/MridulS> and I at least are in agreement
that following black conventions for whitespace is a good ida.
econ-ark/governance#5 <econ-ark/governance#5>
Since there is now the pre-commit hook installed ( see #703
<#703> ) I think that creates de
facto pressure to standardize this way.
Running the black on some of the changed files in this PR, I see that
indeed the tool does some reformatting.
I'm not certain but I believe some of these reformattings are ones you may
have reverted in this PR.
It would be extremely silly if some of us used a reformatting tool as part
of our contributions, and some of us reverted them back to a different
format whenever they worked on the code.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#757 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFNRQHQ55NXKUWLT6JTR3SC2LANCNFSM4OXNUIDQ>
.
|
|
Hmm. Then maybe using the Dolo idiom of a I'm not wedded to it being a Distribution subclass. But I think it would be better to wrap this construct in an object rather than using a generic data type like a list because there is functionality over it that could be encapsulated by a method (for example, reseting all of them at once) |
|
Early bird catches the worm |
One of our DemARKs was malfunctioning because our simulator wasn't replicating the same sequence of random shocks after initializeSim() was called. This PR fixes that problem in all of our models (I think) by implementing extensions to AgentType.resetRNG() that reset the RNG of each discrete distribution used by that type.