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

Add a method (empty) for preSolve called checkRestrictions that can b… #324

Merged
merged 4 commits into from
Jul 3, 2019

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Jun 20, 2019

…e overwritten in classes inheriting from AgentType to check for illegal parameter values.

Fixes #204 by providing infrastructure for checking parameter restrictions.

This way of calling AgentType.preSolve(self) should be fine right @mnwhite ? Currently, all the classes that defined a preSolve failed to check for length time varying things for example, unless AgentType.preSolve was called somewhere I cannot see. The idea is basically that if you have any important restrictions such as values of CRRA, DiscFac, or something else, you write your own checkRestrictions method and raise an exception if the input fails to meet the requirements.

This way still allow users to construct the models, checkRestrictions is simply guarding calls to solve().

…e overwritten in classes inheriting from AgentType to check for illegal parameter values.
@pkofod
Copy link
Contributor Author

pkofod commented Jun 20, 2019

Would you look at that. I caught a mistake thanks to the initialization tests :) (I forgot to import AgentType)

@llorracc
Copy link
Collaborator

@pkofod Thanks for adding this. But it looks like you changed two things at once: Both this and the PR that I just merged modified the same place in IndShockConsumerType, so it is giving a "conflict" error.

If you could resolve the conflicts and let me know, I will merge.

@pkofod
Copy link
Contributor Author

pkofod commented Jun 21, 2019

If you could resolve the conflicts and let me know, I will merge.

Ah yes, that happens. I'll resolve the conflicts!

@llorracc llorracc merged commit cd25db1 into econ-ark:master Jul 3, 2019
@pkofod pkofod deleted the checkrestrictions branch July 3, 2019 19:27
@shaunagm
Copy link
Contributor

Can you add a release note, @pkofod?

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.

Restricting parameter space (throwing errors, warning, etcs)
3 participants