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

Equations in Models #1292

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Equations in Models #1292

wants to merge 12 commits into from

Conversation

sbenthall
Copy link
Contributor

@sbenthall sbenthall commented Jun 27, 2023

This PR is aimed at several interrelated issues, including: #479 #857 and #889

The idea is to make the definition of the mathematical relationships between variables, 'equations' logically separate from the operational code that checks conditions, solves problems, and runs simulations.

Currently, the mathematical aspects of the model are blended in with the operational code. We've been trying to get away from this for some time.

This PR has one solution and a partial demonstration of how it would work. The Model class takes an equations argument, which is a dictionary. The equations are then used as appropriate. I've demonstrated how this refactoring could work for the assignments of thorn and aNrm.

This is just one way to do it. Here are two considerations:

  • It wouldn't be too hard to write a formula resolver, so that resolve('thorn') would:
    • look up the equation for thorn
    • know from its arguments that it needs the parameters dictionary as input
    • pass in that dictionary, and
    • get return the value. (Or a dictionary, with thorn assigned to the value).
  • Ultimately, we want these equations to be composable and built in such a way that they can be compiled by JAX. Maybe a PyTenson implementation would be more efficient down the line. But it would be harder to refactor the current HARK code to do this without doing the more basic Python version first.
  • Tests for new functionality/models or Tests to reproduce the bug-fix in code.
  • Updated documentation of features that add new functionality.
  • Update CHANGELOG.md with major/minor changes.

@sbenthall
Copy link
Contributor Author

Linking to Dolo documentation about the Model for inspiration here:
https://dolo.readthedocs.io/en/latest/model_api.html

@sbenthall sbenthall mentioned this pull request Jun 28, 2023
3 tasks
@sbenthall
Copy link
Contributor Author

Also for reference...

I different way to implement a representation of mathematical equations has already been committed in the frame module. In that work, the equations (called Frame in that code, but really I should rename that to Equation or remove frames entirely in favor of whatever system replaces is) are objects that include a target value, the arguments to its equation, the equation itself (as a Python function), and/or a flag to say if the target value is a control variable (needing a decision rule to be fully specified):

https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsPortfolioFrameModel.py#L114-L231

The equations get evaluated in simulation in the scope of the current state/shock/parameter values:

https://github.com/econ-ark/HARK/blob/master/HARK/frame.py#L591-L670

At the time I wrote that, I had no idea how to integrate equations like these into the solver. #1283 provides a way to do that.

But also, there are pros and cons to the implementation in frame.py and the version currently in this PR is an alternative.

@sbenthall
Copy link
Contributor Author

The failing tests in the ConsAggShockModels are due to the fact that the AggShockModel constructor does not call the constructor of its superclass (IndShockConsumerType), but rather the AgentType base class constructor.

https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsAggShockModel.py#L135-L156

This is not how other model inheritance in HARK works, where the superclass is called.

Is this intentional? Or just an oversight in how the ConsAggShockModel works? if the latter, I can easily fix it in this PR.

@sbenthall sbenthall requested a review from mnwhite July 5, 2023 14:58
@mnwhite
Copy link
Contributor

mnwhite commented Jul 5, 2023 via email

@sbenthall
Copy link
Contributor Author

Let's try changing the inheritance and see if it runs properly.

Ok, I changed it locally and it got tests passing. This change has been committed to this PR.

@sbenthall sbenthall mentioned this pull request Jul 12, 2023
3 tasks
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 96.72% and project coverage change: +0.06 🎉

Comparison is base (2209109) 72.55% compared to head (97a1740) 72.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1292      +/-   ##
==========================================
+ Coverage   72.55%   72.61%   +0.06%     
==========================================
  Files          78       80       +2     
  Lines       13009    13038      +29     
==========================================
+ Hits         9439     9468      +29     
  Misses       3570     3570              
Impacted Files Coverage Δ
HARK/model.py 94.11% <94.11%> (ø)
HARK/ConsumptionSaving/ConsAggShockModel.py 89.68% <100.00%> (ø)
HARK/ConsumptionSaving/ConsIndShockModel.py 87.10% <100.00%> (+0.05%) ⬆️
HARK/core.py 88.23% <100.00%> (-0.18%) ⬇️
HARK/tests/test_model.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sbenthall
Copy link
Contributor Author

Tests are now passing. KinkedR had a similar issue with inheritance.

The substance of this PR is now in and ready for review. Requesting review from @mnwhite

I know I need to do tests, docs, and changelog, and will start on that next.

@sbenthall sbenthall changed the title [WIP] Equations in Models Equations in Models Jul 12, 2023
@llorracc
Copy link
Collaborator

Not sure why this just popped up in my inbox, but it prompted me to look again.

I think thinking about all this is premature. It will really need to be integrated with whatever structure gets created from Pablo's architecture. At most, at this point we could perhaps fruitfully build on what is already in dolo or nolib.

@sbenthall
Copy link
Contributor Author

It's unfortunate that you've been missing the regular HARK meetings @llorracc I think we're quite resolved that this is a good idea.

Essentially, no matter what Pablo's architecture is, the HARK models will need to be moved in this sort of direction in order to integrate with that. This is a necessary incremental improvement to HARK no matter what.

@sbenthall
Copy link
Contributor Author

I'd like to revisit this PR, which is now almost a year old, and perhaps out of date with respect to recent HARK changes. But it was trying to accomplish something that we are still trying to move towards.

The main point of this PR was to try attaching dynamics to the existing ConsumptionSaving models, so that they could be rewritten with a single source of truth about their model equations.

These dynamics, in this PR, looked like this:

IndShockConsumerType_dynamics = {
    **PerfForesightConsumerType_dynamics,
    'G' : lambda gamma, psi : gamma * psi,
    'Rnrm' : lambda R, G : R / G,
    'bNrm' : lambda Rnrm, aNrm : Rnrm * aNrm,
    'mNrm' : lambda bNrm, theta : bNrm + theta,
    'cNrm' : Control(['mNrm']),
    'aNrm' : lambda mNrm, cNrm : mNrm - cNrm
}

This is now essentially the same thing as what's included in the 'block' based consumption model, which was recently merged in:

https://github.com/econ-ark/HARK/blob/master/HARK/models/consumer.py#L26-L44

Now that there's a compact format for model equations already in HARK, it would be a good idea to implement a solution to #1346 and put the equations onto the old models.

@llorracc
Copy link
Collaborator

llorracc commented Jun 2, 2024 via email

@sbenthall
Copy link
Contributor Author

sbenthall commented Jun 2, 2024 via email

@sbenthall
Copy link
Contributor Author

sbenthall commented Jun 2, 2024 via email

@llorracc
Copy link
Collaborator

llorracc commented Jun 2, 2024 via email

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.

3 participants