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

Separate FrameModel from FrameAgentType #1117

Merged
merged 16 commits into from
Mar 9, 2022
Merged

Conversation

sbenthall
Copy link
Contributor

@sbenthall sbenthall commented Feb 22, 2022

This PR makes a number of changes to the "Frame" code in order to improve its flexibility as a way of defining models. A few highlights from the work:

  • A FrameModel class, which holds the Frames and parameters, but has no solution or simulation functionality. (See Disentangle solution and simulation frameworks in HARK.core and downstream classes #495)
    • This class keeps track of whether or not it is an infinite model. (This is akin to the cycles parameter, but a little less confusing).
    • FrameModels can be repeated and combined. This is building towards a more flexible way of composing problems from subproblems -- the subproblems can be built as FrameModels and then glued together.
  • A FrameSet data structure, specifically for containing multiple frames. This solves the problem that frames may have multiple variables as targets, but need to be looked up by single variables and index/order sometimes. An extension of OrderedDictionary.
  • FrameAgentType now takes a FrameModel as input and is designed to handle the simulation functionality in a general way, abstracted over FrameModel.
  • control frames no longer take a transition function. Instead, they get a standard transition function that looks up a decision_rule for the frame target.
  • ConsPortfolioFrameModel has been adapted so now, when the model is solve()ed, the HARK solution object is reformed into decision rules that the simulation code can use.
  • The method for visualization a FrameModel as an influence diagram has been moved to the library as draw_frame_model()

This PR has some new tests for the FrameModel functionality.

Requesting a review from @Mv77 .

  • 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 sbenthall requested a review from Mv77 February 22, 2022 19:10
@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2022

Codecov Report

Merging #1117 (1d122b2) into master (ea73795) will increase coverage by 0.12%.
The diff coverage is 96.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
+ Coverage   73.83%   73.95%   +0.12%     
==========================================
  Files          69       70       +1     
  Lines       10636    10759     +123     
==========================================
+ Hits         7853     7957     +104     
- Misses       2783     2802      +19     
Impacted Files Coverage Δ
HARK/distribution.py 83.33% <ø> (ø)
HARK/tests/test_core.py 100.00% <ø> (ø)
HARK/frame.py 84.89% <94.69%> (-7.59%) ⬇️
HARK/ConsumptionSaving/ConsPortfolioFrameModel.py 100.00% <100.00%> (+4.76%) ⬆️
...mptionSaving/tests/test_ConsPortfolioFrameModel.py 100.00% <100.00%> (ø)
HARK/tests/test_frame.py 100.00% <100.00%> (ø)
HARK/ConsumptionSaving/ConsIndShockModel.py 85.82% <0.00%> (ø)
HARK/core.py 91.47% <0.00%> (+1.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea73795...1d122b2. Read the comment docs.

@sbenthall
Copy link
Contributor Author

The graph layout algorithm that is most useful for rendering influence diagrams is available in networkx via the pydot package, which is a wrapper around the C package graphviz.

Unfortunately, it is not easy to automatically install graphviz in a cross-platform way, and this is causing some of the notebooks to fail.

I'll fix this to get the tests to pass.

@sbenthall
Copy link
Contributor Author

Ok, tests pass now. It's ready for review.

Copy link
Contributor

@Mv77 Mv77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbenthall,

I just went through this.

I must confess that I do not fully understand the specifics (context, scope, references) of the architecture in place, or its advancements, so I am unable to give specific feedback on that for now.

However, I was able to see the general purposes of the PR and I like what I see in terms of making things modular, allowing models to be integrated, visualization tools, infrastructure for solvers ... etc

So I don't get the specifics, but I like the general things that I see. I approve.

@sbenthall
Copy link
Contributor Author

Thank you, @Mv77 . That's good feedback. I surely need to provide better documentation for what's going on here. I will see what I can do about that before merging.

@sbenthall sbenthall merged commit 17fd582 into econ-ark:master Mar 9, 2022
nicksawhney pushed a commit to nicksawhney/HARK that referenced this pull request May 27, 2022
@sbenthall sbenthall added this to the 0.13.0 milestone Jan 4, 2023
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