Update check_conditions() #1294
Conversation
ConsIndShockSolver is always chosen now, and calc_bounding_values() works with new distribution format.
Perfect foresight model now produces a conditions_report field in addition to logging. Might need to check on the spacing with logging.
Not done yet. Also fixed a couple small mistakes in PF.
Still need to write "verbose" comments for check_conditions.
Still lacks Harmenberg growth patience factor/condition, as well as comments about the Modigliani mortality adjusted GIC. Will look at paper more closely.
There were a few typos. Violating FVAC but not WRIC now produces an ambiguous message, as the solution only *might* not exist.
Old add_stable_points methods in the PF and IndShock solvers were being run *every* period, which is very costly and essentially pointless. The new method lives on the IndShockConsumerType class and checks for relevant conditions before evaluating. I need to double check, but I think the solution was accelerated significantly by getting rid of that code.
Search for mNrmTrg even if GICMod fails. Also put mNrmBal and mNrmTrg into top level. Need to change later. These changes were made while updating the BST dashboard notebook to be compatible.
The conditions_report is now sent to _log.info if not quiet. ConsIndShock.post_solve will now run calc_stable_points() if appropriate, so that the target and balanced-growth mNrm levels are added to the solution. At the default parameters (infinite horizon), the main branch takes 0.328 seconds to solve on my computer. After moving the calc_stable_points code outside of the solver loop and only running it at the end (if appropriate), it takes 0.100 seconds to solve. Over two-thirds of solution time was being *wasted* on calc_stable_points, whose work is not used during the solution and *could not* plausibly be used.
|
This PR makes me happy! Would it be possible to make a separate PR for the movement of the stable points calculation? My main comment on the check_conditions work, which is not meant to be a blocker, but rather as something more aspirational, is that really check_conditions depends on a lot of 'data'
I think that in an ideal world, none of this data would be hard-coded into the model. Rather, these can be configuration objects which are treated with generic modeling functionality. I'm thinking of how to move in that direction in #1292 and would like to come up with some system we all feel good about. |
|
check_conditions is very, very specific to the PF model and the ConsIndShock model. It could be plausibly extended to KinkedR and maybe ConsMarkovModel with more theory work, but it's a heavy lift to go beyond that. I don't think we want to have big code structures or concepts build around check_conditions, because we can only go so far with it-- it's not a general concept, in my opinion. As for separating the changes to calc_stable_points from the check_conditions stuff... probably, but I'd need to undo work here. It looks like I put all of the "real" work for that into one commit, so maybe it's feasible. Tests are failing right now because I renamed mNrmStE to mNrmBal. What does StE mean? Steady expectations? It's referred to as the "balanced growth" point in BST, so I changed it to Bal. I suppose I should keep the name for the sake of the tests, and then in a separate PR later change only the variable name. |
The issue with the current architecture is that while these conditions are specific to the ConsIndShock model, because the ConsIndShock model is the superclass of all other consumption models, we wind up with very heavy, model-specific conditions code in every downstream model, even when it's entirely inappropriate. This is the worst of all worlds. Making the condition checking code more generic improves that somewhat. Some aspects of the problem, such as wanting printable descriptions for model parameters, may be genuinely general. But maybe you are right that checking conditions is so model specific that it should not be in the AgentType (or subclasses of it) code at all. In that case, I'd recommend that the conditions-checking functionality live in its own module, and take configuration data and the model as arguments. |
|
Yes, downstream models will need to overwrite check_conditions() to simply
pass, else they will break or generate unexpected results. The
conditions-checking code isn't "heavy" in the sense of being
computationally intensive, but it is meaningless outside of the context of
that specific model.
But I *don't* think it should live outside of the AgentType subclasses. One
of the improvements that should / will be made is to make details of the
solution method depend on (at least some) of the conditions. As-is, the
consumption function is specified to extrapolate as an exponential decay
toward the limiting linear perfect foresight solution. In some cases, that
limiting linear solution *does not exist*. In the specific case where we
can predict that situation, the solver needs to be told "don't extrapolate
like that, extrapolate like *this* instead". Except no one actually knows
what "*this*" is yet.
…On Wed, Jun 28, 2023 at 11:24 AM Sebastian Benthall < ***@***.***> wrote:
check_conditions is very, very specific to the PF model and the
ConsIndShock model. It could be plausibly extended to KinkedR and *maybe*
ConsMarkovModel with more theory work, but it's a heavy lift to go beyond
that. I don't think we want to have big code structures or concepts build
around check_conditions, because we can only go so far with it-- it's not a
general concept, in my opinion.
The issue with the current architecture is that while these conditions are
specific to the ConsIndShock model, because the ConsIndShock model is the
superclass of all other consumption models, we wind up with very heavy,
model-specific conditions code in every downstream model, even when it's
entirely inappropriate. This is the worst of all worlds.
Making the condition checking code more generic improves that somewhat.
Some aspects of the problem, such as wanting printable descriptions for
model parameters, may be genuinely general.
But maybe you are right that checking conditions is so model specific that
it should not be in the AgentType (or subclasses of it) code at all. In
that case, I'd recommend that the conditions-checking functionality live in
its own module, and take configuration data and the model as arguments.
—
Reply to this email directly, view it on GitHub
<#1294 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFI73OEEHXCLDVHOIFTXNREDXANCNFSM6AAAAAAZXHWSMU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
So, given a model These values
What I've described is something general -- the ability to define derivative mathematical objects in terms of more basic parameters. Dolo implements something like this generically, calling it |
|
One part in particular that looks like useful general functionality that need not be hard-coded into a model is this part: I understand that you don't have the appetite for generalizing out this sort of functionality. I may get to it once the PR is merged. If you can keep that under consideration as you finish your implementation, that might make the transition towards separating models, solvers, and simulators from each other, which we discussed the other week, go smoother. As is, the |
Per SB's request, I'm splitting the change to calc_stable_points to be in a separate PR. This commit reverts the changes from a prior commit and should make the tests run properly now (because mNrmStE has not been renamed).
|
I just stripped out the changes to where "stable points" are calculated, so the tests should pass now. I'm 95% confident that the only failure mode was the renaming for mNrmStE to mNrmBal. I still need to go double check some of the messages and make sure I didn't leave any "...I don't know what will happen, this is weird" stuff. The CHANGELOG should also be updated. Oh, AND: @sbenthall What do you want to call the dictionary that holds auxiliary factors / information that never needs to be used by the solver, but is useful to have around? As-is, I put a lot more crap at the top level of the AgentType, but that should be fixed before merging. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1294 +/- ##
==========================================
+ Coverage 72.71% 72.82% +0.11%
==========================================
Files 78 78
Lines 13057 13228 +171
==========================================
+ Hits 9494 9633 +139
- Misses 3563 3595 +32
☔ View full report in Codecov by Sentry. |
That's a good question. 'auxiliary' is not an obvious label to me. 'definitions'? Maybe @llorracc has an idea? |
|
@mnwhite status of this? |
|
If we can settle on a name for the dictionary where all the various
patience factors (etc) should live, I can pack everything into that and
this should be good to go. Then I can (slightly) update the PR that moves
the stable points code and that will be ready.
…On Wed, Jul 26, 2023 at 2:11 PM Sebastian Benthall ***@***.***> wrote:
@mnwhite <https://github.com/mnwhite> status of this?
—
Reply to this email directly, view it on GitHub
<#1294 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADKRAFM4WCQSPY7B26UWAILXSFMWFANCNFSM6AAAAAAZXHWSMU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
A new dictionary has been added to PerfForesightConsumerType and IndShockConsumerType, called auxiliary. It contains all the various patience factors and other semi-useful values that are constructed for check_conditions, but aren't needed to solve (nor simulate) the model. This dictionary can be renamed with a simple replace-all on self.auxiliary.
Patient factors and conditions report now life in the bilt dictionary.
|
This PR is now complete, as far as I can tell @sbenthall . It looks like the Python 3.10 test might be timing out on MacOS only, but we'll see. |
| ] # cycle time has already been advanced | ||
| # Cycle time has already been advanced | ||
| self.shocks["PermShk"] = PermGroFac[self.t_cycle - 1] | ||
| #self.shocks["PermShk"][self.t_cycle == 0] = 1. # Add this at some point |
There was a problem hiding this comment.
I'm confused about this comment...
It looks like the main change here is just where the line breaks are.
If you use a code formatter like black does it revert this?
| param_desc : str | ||
| Description of parameters as a unicode string. | ||
| ''' | ||
| params_to_describe = [ |
| ['PermGroFac', 'permanent income growth factor', 'G',True], | ||
| ['CRRA', 'coefficient of relative risk aversion','ρ',False], | ||
| ['LivPrb', 'survival probability','ℒ',True], | ||
| ['APFac', 'absolute patience factor', 'Þ=(βℒR)^(1/ρ)',False] |
There was a problem hiding this comment.
conceptually:
- these are probably more like tuples than lists
- you have an implicit type here: (string, string, string, bool)
- this is really data about how the model is configured. Rather than putting it inside the method, it would be easier to (a) modify it and (b) change it in subclasses if this was stored in a data structure outside of the method.
| @@ -1609,7 +1611,7 @@ def __init__(self, verbose=1, quiet=False, **kwds): | |||
| self.quiet = quiet | |||
| self.solve_one_period = make_one_period_oo_solver(ConsPerfForesightSolver) | |||
| set_verbosity_level((4 - verbose) * 10) | |||
|
|
|||
| self.bilt = {} | |||
There was a problem hiding this comment.
I believe that in the "HARK 2.0 alpha" PR, CDC's use of mispelt English words was to make it easier to find-and-replace them all with a more readable, less opaque name.
Is this being used for anything besides conditions? I would be happy to name this conditions. Or could be built. Or derived.
| for j in range(len(params_to_describe)): | ||
| this_entry = params_to_describe[j] | ||
| if this_entry[3]: | ||
| val = getattr(self,this_entry[0])[0] |
There was a problem hiding this comment.
I believe that we now have an alternative way to get at a model's parameters.
I think we are currently storing the parameters dictionary as a dictionary, so that we can pull params from it, rather than relying on getattr to pull them from the object. The latter is the "whiteboard problem", and while this still works, I think we've implicitly deprecated this style.
So... consider self.parameters[this_entry[0]]
| @@ -3071,120 +3163,239 @@ def pre_solve(self): | |||
| self.update_solution_terminal() | |||
| if not self.quiet: | |||
| self.check_conditions(verbose=self.verbose) | |||
|
|
|||
|
|
|||
| def describe_parameters(self): | |||
There was a problem hiding this comment.
Confused about what this empty method is doing here.
| val = getattr(self,this_entry[0]) | ||
| except: | ||
| val = self.bilt[this_entry[0]] | ||
| this_line = this_entry[2] + f'={val:.5f} : ' + this_entry[1] + ' (' + this_entry[0] + ')\n' |
There was a problem hiding this comment.
This way of constructing a string from these 'entries' is repeated often.
This can be functionalized out.
|
Review is inline. Also see the merge conflict on the CHANGELOG. |
With log utility, the expression for the utility-compensated permanent shock (and thus the value of autarky factor) would break with a divide by zero error. This is now fixed, with the end result that the VAFac is just beta with log utility.
|
I just fixed the last (very tiny) outstanding issue with this, so it's ready to merge, assuming checks pass. Exactly one check failed (for one version of Python?) on the previous commit, which only merged in recent changes from master. Not sure what's going on, but I've seen timeout issues before. |
|
This is ready to merge @alanlujan91 |
|
Ugh, I just realized that I missed / never saw all of @sbenthall 's review comments. Literally none of them, until just now when I got the notification that Alan merged it. To address them:
|
This PR incorporates CDC's work on the branch BST-HARK-pre-release-v4, at least with respect to the check_conditions() method for PerfForesightConsumerType and IndShockConsumerType. The new code structures in his branch has not been incorporated, as it represents a large overhaul of HARK. Instead, the condition-checking code has been greatly simplified, but also expanded to be more reader-friendly.
The check_conditions() method is now called automatically as part of pre_solve. This method fills in the attribute conditions_report, which is a string explaining parameter values, various patience factors (and their associated conditions), and an explanation of what the conditions mean jointly when verbose is True. A few of those messages need editing or verifying, so this PR is not yet ready to merge.
This PR also moves the calc_stable_points functionality out of the solver and into IndShockConsumerType. This change accelerates the solver by a factor of ~3.5x, and there was literally zero value to running calc_stable_points every single period during backward solution. The target (and balanced-growth) market resources ratio is not interesting, relevant, or meaningful outside of the infinite horizon, single repeated period case... and it doesn't mean anything until the solution has converged. This functionality is now run automatically as part of of post_solve when appropriate.
The other remaining work item is that all of the various factors that are computed are stored as attributes of the agent, which is not something we want to do anymore. These should be in a dictionary (like history or conditions), but I need feedback on what that dictionary should be called. Maybe factors? It's called Bilt in CDC's dev branch, but I'm not really a fan of that.
No new tests have been added, and I don't think they need to be. The changelog has not been updated, but needs to be.