Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constrained perfect foresight #299

Merged
merged 14 commits into from
Oct 19, 2019
Merged

Constrained perfect foresight #299

merged 14 commits into from
Oct 19, 2019

Conversation

mnwhite
Copy link
Contributor

@mnwhite mnwhite commented May 31, 2019

Add liquidity constraint (BoroCnstArt) to the perfect foresight consumption-saving model. Doesn't calculate value function if BoroCnstArt is not None, as we don't have the property that vNvrsFunc is piecewise linear. True value function requires calculating value at a bunch of points and then constructing as usual. This would be additional computation (and would require more parameters), so I didn't write this bit.

mnwhite added 2 commits May 30, 2019 23:34
PerfForesightConsumerType can now handle an artificial borrowing constraint, like in all our other models.  Currently doesn't calculate value function if BoroCnstArt is not None, as this requires additional work to compute.

When this is merged, it will be a breaking change for all existing instances of PerfForesightConsumerType, as they will now require BoroCnstArt and aXtraCount to be attributes.
The "universal distance metric" had a bug in one line that had never come up.  If the two objects being compared have a shape, but the shapes are not the same, then it tried to take the supnorm between them; this fails because arithmetic is not defined on tuples.  Fix simply converts the shape tuples into arrays.
Forgot that I eliminated the redundant mNrmMin (only use mNrmMinNow).  Now fixed.
@mnwhite
Copy link
Contributor Author

mnwhite commented May 31, 2019

Okay, automated testing is pretty cool. It alerted me to a (stupid) typo I made at the last minute by failing to manually test a case.

Apparently Py3 doesn't like to compare float and None, but Py2 is fine with this (and is correct: None is neither greater than, equal to, or less than any real number).  Now fixed.
@mnwhite
Copy link
Contributor Author

mnwhite commented May 31, 2019

Even cooler: It flagged something that works in Py2 but not Py3. Very neat.

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 2, 2019

The model notebook in DEMARK has been updated to account for this change (in a PR), but it should only be merged once this PR is merged and we do a release. Our DEMARKs are pointing to the latest public release of HARK, correct?

@llorracc
Copy link
Collaborator

llorracc commented Jun 2, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 8, 2019

Just looked through the code -- it looks good (though haven't tested until we resolve whether this should be a "breaking change". I don't want to do the thing of including a custom version of ConsIndShockModel.py in a notebook until we make the new release.

But one question: Don't see why the aXtraGrid parameter is necessary. The extra gridpoints are constructed on the fly, and there'll be one of them added for each iteration, so the user doesn't need to specify that number in advance, just needs to specify how many periods back to solve, right? In fact, if aXtraGrid is fixed and they specify that the problem should be solved one more period, there's no way to do that without adding more gridpoints?

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@llorracc-git
Copy link
Contributor

llorracc-git commented Jun 8, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 8, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 8, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 8, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 8, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 8, 2019 via email

@shaunagm
Copy link
Contributor

Sounds like you've worked out a plan here but some $.02 from me...

Yes, but I want to SHARE this notebook with referees/editors/the universe.
If I could impose a requirements "econ-ark==" whatever in which I could
guarantee it would work, then that might be enough. To do that we'd need
to create a new dev branch (containing the breaking changes) which will
exist in perpetuity that my code can insist upon.

Unfortunately how we've got our notebooks set up now, they all use the same requirements.txt file, so you can only pin to one version across all notebooks. However in the long run we should definitely change that, and in the short run you can just do the pip install econ-ark==$version from the notebook itself.

This is not great practice, but a short term fix when you're not certain how you want to implement a feature is to implement it one way in a dev release and then revert or change it in the next dev release. Practically speaking it's not that different from when you accidentally release a bug in a dev release and revert the change in the next release. It's a little annoying for dev release users, but your dev release users are the subset of users who've actively selected into being annoyed in exchange for access to the latest features.

@llorracc
Copy link
Collaborator

Good point, notebooks get their versions from requirements.txt if they are being run by mybinder. So a good temporary solution might just be to pip install econ-ark==[] whatever on the dev branch and then revisit this when we are ready to do the next stable release.

Matt, I hope changing this to be a non-breaking change is super easy?

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 10, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 10, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 10, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 10, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 10, 2019 via email

@shaunagm
Copy link
Contributor

I don't know what a "dev branch" would even be. All branches other than master are arguably development branches.

I assume he meant to say "dev release", but please confirm that Chris.

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 10, 2019 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 10, 2019 via email

@llorracc-git
Copy link
Contributor

llorracc-git commented Jun 10, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 10, 2019 via email

@llorracc-git
Copy link
Contributor

llorracc-git commented Jun 10, 2019 via email

mnwhite added 4 commits June 12, 2019 23:01
Renamed model input to be more direct, rather than use name for similar (but not same) thing in other models.
If BoroCnstArt is not specified in PerfForesight model, preSolve assumes user meant no artificial borrowing constraint (as the model has always been until now).  If BoroCnstArt is specified (not None) *and* it's an infinite horizon model *and* MaxKinks is not specified, then an AttributeError is raised to notify the user.
Changes to PerfForesightModel required changes to test comparing PF and IndShock solutions when they should be the same.  Test now specifies a dictionary rather than using deepcopy and swapping the solution method.
@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 13, 2019

I've made the changes discussed here. All dictionaries that previously worked for PerfForesightConsumerType will still work now; if they don't specify BoroCnstArt, we assume they intended to use the old model where no such thing could exist (and thus set it to None). If the user tries to specify BoroCnstArt, do an infinite horizon model, but not set MaxKinks, the code raises an AttributeError telling them this explicitly.

Also, the thing I was calling aXtraCount (to not make a new thing that is almost-but-not-quite the same as aXtraCount) is now MaxKinks.

This branch should be ready to merge (please don't do a squash and merge).

@mnwhite
Copy link
Contributor Author

mnwhite commented Jun 28, 2019

@llorracc I think you wanted this for one of your papers, and it's been ready to merge for two weeks.

@llorracc
Copy link
Collaborator

@mnwhite, I'm at the point of wanting this again, but see there is a merge conflict that you can probably fix in 30 sec. Could you take a look?

@mnwhite
Copy link
Contributor Author

mnwhite commented Oct 17, 2019 via email

@mnwhite
Copy link
Contributor Author

mnwhite commented Oct 17, 2019

This PR is now broken because of other work that was done on ConsIndShockModel.py in the four months since this model extension was written and PR submitted. Attempting to fix.

Some variable names were updated in the solver while this PR was outstanding; now fixed.
@mnwhite
Copy link
Contributor Author

mnwhite commented Oct 17, 2019

Okay, it runs again and all checks pass. If you want this feature added, merge soon.

@llorracc llorracc merged commit 89f24b6 into master Oct 19, 2019
@llorracc
Copy link
Collaborator

Release note: Added constrained perfect foresight model solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants