MNW cleanup May 2020 #698
Merged
MNW cleanup May 2020 #698
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
This example model was out of date with current HARK format; now fixed. Also tweaked a few comments.
Update the aggregate shocks example so that it uses the new distribution style. Also adjust some comments in various files (and improve commented-out plots in ConsAggShock).
Fix small issues with FashionVictimModel
sbenthall
reviewed
May 27, 2020
| #plt.xlabel('log(MaggNow)') | ||
| #plt.ylabel('log(AaggNow)') | ||
| #plt.plot(logMagg[plot_start:],logAagg[plot_start:],'.k') | ||
| #plt.show() |
There was a problem hiding this comment.
If your goal is to 'clean up" this code, I'd suggest that removing commented code blocks would make the code 'cleaner'.
sbenthall
reviewed
May 27, 2020
| # plt.plot(logMagg[these],logAagg[these],'.') | ||
| if verbose: | ||
| #plt.plot(logMagg[these],logAagg[these],'.') | ||
| pass |
There was a problem hiding this comment.
I don't understand why our core library code should have a condition that does nothingl
sbenthall
reviewed
May 27, 2020
| [np.array([0.96, 0.04]), np.array([1.0, 1.0]), np.array([1.0 / 0.96, 0.0])], | ||
| [np.array([0.90, 0.10]), np.array([1.0, 1.0]), np.array([1.0 / 0.90, 0.0])], | ||
| DiscreteDistribution(np.array([0.96, 0.04]), [np.array([1.0, 1.0]), np.array([1.0 / 0.96, 0.0])]), | ||
| DiscreteDistribution(np.array([0.90, 0.10]), [np.array([1.0, 1.0]), np.array([1.0 / 0.90, 0.0])]), |
|
Ok, we can delete the commented out code. Uncommenting prints figures to
screen while the economy solves; I suppose we could just add another
boolean flag to allow that.
…On Wed, May 27, 2020 at 8:21 AM Sebastian Benthall ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/ConsumptionSaving/example_ConsAggShockModel.py
<#698 (comment)>:
> @@ -178,28 +173,26 @@ def mystr(number):
# NOTE: These agents aren't exactly like KS, as they don't have serially correlated unemployment
KSexampleType = deepcopy(AggShockMrkvExample)
KSexampleType.IncomeDstn[0] = [
- [np.array([0.96, 0.04]), np.array([1.0, 1.0]), np.array([1.0 / 0.96, 0.0])],
- [np.array([0.90, 0.10]), np.array([1.0, 1.0]), np.array([1.0 / 0.90, 0.0])],
+ DiscreteDistribution(np.array([0.96, 0.04]), [np.array([1.0, 1.0]), np.array([1.0 / 0.96, 0.0])]),
+ DiscreteDistribution(np.array([0.90, 0.10]), [np.array([1.0, 1.0]), np.array([1.0 / 0.90, 0.0])]),
i love this.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#698 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFLYBCP7SQDSRIXROGLRTUAV7ANCNFSM4NKMN4AQ>
.
|
|
What do you love about this? I don't get it.
…On Wed, May 27, 2020 at 8:21 AM Sebastian Benthall ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/ConsumptionSaving/example_ConsAggShockModel.py
<#698 (comment)>:
> @@ -178,28 +173,26 @@ def mystr(number):
# NOTE: These agents aren't exactly like KS, as they don't have serially correlated unemployment
KSexampleType = deepcopy(AggShockMrkvExample)
KSexampleType.IncomeDstn[0] = [
- [np.array([0.96, 0.04]), np.array([1.0, 1.0]), np.array([1.0 / 0.96, 0.0])],
- [np.array([0.90, 0.10]), np.array([1.0, 1.0]), np.array([1.0 / 0.90, 0.0])],
+ DiscreteDistribution(np.array([0.96, 0.04]), [np.array([1.0, 1.0]), np.array([1.0 / 0.96, 0.0])]),
+ DiscreteDistribution(np.array([0.90, 0.10]), [np.array([1.0, 1.0]), np.array([1.0 / 0.90, 0.0])]),
i love this.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#698 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFLYBCP7SQDSRIXROGLRTUAV7ANCNFSM4NKMN4AQ>
.
|
|
Just a positive vibe. I'm not sure why Travis failed a build on this. I would rather merge this, then #702, as that's better PR hygiene. |
"Progress plots" of M vs A plots in the aggregate shocks model now only display if the economy has a 'plot_verbose' attribute set.
sbenthall
reviewed
May 28, 2020
| if verbose: | ||
| #plt.plot(logMagg[these],logAagg[these],'.') | ||
| pass | ||
| if hasattr(self, 'plot_verbose'): |
There was a problem hiding this comment.
now the PR inconsistently uses 'plot_verbose' or 'verbose_plot' for this feature.
I'm thumbs down on:
- adding undocumented model parameters
- the model specific controls of verbosity, discussed in make HARK less "verbose" -- introduce logger #701
But if this is necessary functionality, then maybe we can wire it through the "view" code in a way that's clean.
|
Crap, that was a dumb typo.
Can the logger get graphical output?
…On Thu, May 28, 2020 at 8:36 AM Sebastian Benthall ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In HARK/ConsumptionSaving/ConsAggShockModel.py
<#698 (comment)>:
> @@ -1725,9 +1728,8 @@ def calcAFunc(self, MaggNow, AaggNow):
for i in range(self.MrkvArray.shape[0]):
these = i == MrkvHist
slope, intercept, r_value, p_value, std_err = stats.linregress(logMagg[these], logAagg[these])
- if verbose:
- #plt.plot(logMagg[these],logAagg[these],'.')
- pass
+ if hasattr(self, 'plot_verbose'):
now the PR inconsistently uses 'plot_verbose' or 'verbose_plot' for this
feature.
I'm thumbs down on:
- adding undocumented model parameters
- the model specific controls of verbosity, discussed in #701
<#701>
But if this is necessary functionality, then maybe we can wire it through
the "view" code in a way that's clean.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#698 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFPA7IPA2PRTARXJ2Z3RTZLGFANCNFSM4NKMN4AQ>
.
|
Tests for models in ConsAggShockModel.py have been revised and expanded. There are now tests for "micro" and "macro" versions of both the regular and Markov versions. Options have been set so that it "fake solves", with a small number of agents, short history, and simply quitting on solving after 3 loops.
Really sure I googled for this exact method, but failed. Thanks, Shauna!
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.
Various small comment and functionality fixes for ConsAggShockModel examples and FashionVictim.