Fix simulation performance of AggShockMarkovConsumerType #702
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bring in MNW changes
DiscreteDistribution class was implemented awkwardly in AggShockMarkovConsumerType.getShocks(), leading to extremely slow simulation (15x worse performance in relevant example). Simplified shock drawing code, timing issue fixed. Also further cleaned up DiscreteDistribution.drawDiscrete(), as the exact_match version did not work correctly with multiple shock dimensions.
|
This includes my other two recent PRs. See those threads for other explanation. |
|
There are some edits to There are also Ideally, the two are synced up. I'm a little confused at this point as to why they aren't. We've been using jupyter lab/jupytext to keep those working together. |
|
If the performance issue has been fixed, could you also reenable the automated test? |
|
I'll try to do that, yes. But I don't think it was ever enabled? Maybe it
was.
Yes, those are the PRs. That's why there's random stuff like edits to
examples in this PR-- I pulled in MNWcleanup so I could run the relevant
example.
I don't know anything about the notebooks. Are we supposed to generate
notebooks from py files, or vice versa?
…On Wed, May 27, 2020 at 8:09 AM Sebastian Benthall ***@***.***> wrote:
If the performance issue has been fixed, could you also reenable the
automated test?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#702 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFJLTV2LHH7FNDBEKHTRTT7H7ANCNFSM4NLFRQMQ>
.
|
|
Those are supposed to be synced up using jupytext. (Install it using pip install jupytext if not already present)
To sync a paired notebook with a .py file ->
jupytext --sync path_to_python_file.py
… On 27-May-2020, at 6:29 PM, Matthew N. White ***@***.***> wrote:
I'll try to do that, yes. But I don't think it was ever enabled? Maybe it
was.
Yes, those are the PRs. That's why there's random stuff like edits to
examples in this PR-- I pulled in MNWcleanup so I could run the relevant
example.
I don't know anything about the notebooks. Are we supposed to generate
notebooks from py files, or vice versa?
On Wed, May 27, 2020 at 8:09 AM Sebastian Benthall ***@***.***>
wrote:
> If the performance issue has been fixed, could you also reenable the
> automated test?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#702 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ADKRAFJLTV2LHH7FNDBEKHTRTT7H7ANCNFSM4NLFRQMQ>
> .
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#702 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABI5RFHF762Z7UFLDWGAOALRTUFC7ANCNFSM4NLFRQMQ>.
|
Merged
|
Let's put the jupytext thing into the contributor guidelines |
|
We'll need to adjust the target for one AggShock test due to the change in the RNG. This test doesn't fully solve, but instead "quits early", so the distribution of shocks matters for the "result". |
RNG changes in other commit moved a test target; now fixed (hopefully).
|
Tests are now passing, will be ready to merge soon. |
Other PR overwrote changed value.
This reverts commit 15ce7e3.
…o SimPerformance
I thought I just fixed this.
|
This is ready to be merged again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation of DiscreteDistribution in AggShockMarkovConsumerType was incomplete/awkward, leading to extreme performance problems for simulation. Now fixed.
Also removed references to timeFwd() in one of the examples, and maybe a couple other comment or whitespace tweaks that happened to be in my active changes.