Move addStablePoints from solver to post-solve #1349
Conversation
Calculating the "stable points" mNrmTrg and mNrmStE is a costly operation that is not useful in most contexts. This commit moves that code out of the solver and into a method on IndShockConsumerType, which is called as part of post_solve only if it is appropriate. The results get put into both self.bilt and self.solution (for compatibility with tests).
Somehow missed these on the previous commit.
I didn't realize that the unit tests include the "stable points" for a perfect foresight model. This commit moves the methods from IndShockConsumerType to PerfForesightConsumerType.
This was done manually in my own test code, hence why I didn't notice it was missing.
This method is not implemented for KinkedRConsumerType, so I have removed the call to it. Also fixed docstring for the empty method.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1349 +/- ##
==========================================
+ Coverage 71.68% 71.72% +0.03%
==========================================
Files 84 84
Lines 13941 13910 -31
==========================================
- Hits 9994 9977 -17
+ Misses 3947 3933 -14 ☔ View full report in Codecov by Sentry. |
ConsLabeledModel inherits from IndShockConsumerType, but doesn't use cFunc as an attribute of solution, so add_stable_points can't be run for it; skip post_solve. Added missing T_cycle length in one example notebook, which was generating an error with check_conditions().
One of the strings was not being assigned to GIC_message, and instead just left dangling. This should fix many test failures.
|
All tests pass, adding changelog entry. |
|
@sbenthall @alanlujan91 The fixes for this were extremely tiny: Add missing T_cycle definitions in one notebook and add on a missing string declaration in check_conditions-- one of the "GIC_message undefined" errors was because it meant to define a string, but didn't. Should be ready to merge. |
|
looks good to me! |
|
It looks like this was approved but then never merged. In doing some work today, I became confused that the code didn't look the way I expected. I'm going to see if this can be rectified with the current master branch. |
|
sorry, I guess I approved it but was waiting for @sbenthall's review |
|
I apologize for my part in any miscommunication around this. |
This needs to get resolved at some point; it's happening to Alan too. One of these "changes" is adding an extra space before a comment-- two spaces rather than one. Come on.
|
All tests should pass now. I'm having the same issue as Alan: running black locally does not generate the same formatting as the remote's version of black. |
|
@sbenthall This passes tests now and is up to date with master. The only file that's substantively changed is ConsIndShockModel.py; the other three files are tiny compatibility adjustments. |
Calculating the "stable points" mNrmTrg and mNrmStE is a costly operation that is not useful in most contexts. This commit moves that code out of the solver and into a method on IndShockConsumerType, which is called as part of post_solve only if it is appropriate. The results get put into both self.bilt and self.solution (for compatibility with tests).
Please ensure your pull request adheres to the following guidelines: