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

Centralizing Value and MargValue functions #888

Merged
merged 13 commits into from
Jan 7, 2021

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Dec 17, 2020

The trick of working with value and marginal value functions in 'inverse-space' is now applied in many models in HARK. This happens mostly through the ValueFunc and MargValueFunc classes.

However, the general idea of these classes is not captured in a centralized place. Different versions have been defined as models have been added: the 1-d version is in ConsIndShockModel, the 2-d version in ConsGenIncProcessModel, and my new portfolio model will use a 3-d version.

This makes it so that if I'm working on a new, say, 2-d model, I have to go check whether this has already been implemented in some of the existing models. Or if I need both 1-d and 2-d versions (as ConsPortfolioModel does), I need to import them from different models. I think this will lead to multiple versions of these objects being defined in different places.

This pr aims to construct general n-dimensional versions that can be imported from interpolation (or somewhere else?).

Would this be useful or is there a reason for these objects to be defined in different places?

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 17, 2020

The previous commits centralize ValueFunc. All of its uses now point to a single version in interpolation.py.

If you like this idea, I can continue with MargValueFunc, which is trickier as some instances of its use need its derivatives. Still, I think the task is possible and worthy.

@sbenthall
Copy link
Contributor

In my view this is exactly the right direction.

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 19, 2020

The previous commits implement n-dimensional MargValueFunc and MargMargValueFunc, and replace their previous uses and definitions.

@Mv77 Mv77 changed the title [WIP] Centralizing Value and MargValue functions Centralizing Value and MargValue functions Dec 19, 2020
@sbenthall
Copy link
Contributor

Would you like me to review this? I see it is not marked as WIP

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 21, 2020

Yes, please

@sbenthall sbenthall self-assigned this Dec 21, 2020
@sbenthall
Copy link
Contributor

Beautiful work. Thanks for doing this. It passes review.

Could I ask you to please add a commit with an edit to the CHANGELOG describing the change and linking to this PR?

https://github.com/econ-ark/HARK/blob/master/Documentation/CHANGELOG.md

I believe it counts as a Major change. It should make it into the next release, which may be early this week.

@sbenthall
Copy link
Contributor

One other question: I've noticed this related code in the PerfectForesight model which is involved in constructing value functions.

def defValueFuncs(self):

Is it possible to streamline that code any further given the changes you are making here?

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 21, 2020

Is it possible to streamline that code any further given the changes you are making here?

Does not seem to me like it is. There are four lines of code, two using ValueFunc and MargValueFunc (that now will come from interpolation.py), the other two just defining a pair of points that are needed for ValueFunc in the special perfect foresight case.

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 21, 2020

Beautiful work. Thanks for doing this. It passes review.

Could I ask you to please add a commit with an edit to the CHANGELOG describing the change and linking to this PR?

https://github.com/econ-ark/HARK/blob/master/Documentation/CHANGELOG.md

I believe it counts as a Major change. It should make it into the next release, which may be early this week.

Ready! (in the last commit)

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 23, 2020

Call them ___CRRA.

@Mv77
Copy link
Contributor Author

Mv77 commented Dec 27, 2020

Since @sbenthall's review, I have:

  • Added the change to the changelog.
  • Changed the name of the functions to [az]*ValueFuncCRRA, to make it clear that CRRA utility is hardcoded in them.

@sbenthall sbenthall merged commit 2851279 into econ-ark:master Jan 7, 2021
@Mv77 Mv77 deleted the TransformedFuncs branch January 15, 2021 21:04
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.

2 participants