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

ConsPortfolioModel Overhaul #1047

Merged
merged 8 commits into from
Oct 21, 2021
Merged

Conversation

alanlujan91
Copy link
Member

@alanlujan91 alanlujan91 commented Jul 23, 2021

This PR covers a number of changes to ConsPortfolioModel and supporting files:

  • Creates object oriented solver to facilitate using pieces of solution method in other solvers.
  • Updates solution method for joint distribution of income and risky asset shocks (solver now runs, but there are no proper tests for this feature; an easy test is to assume independent distributions are not independent, which should give very similar if not exactly equal results)
  • Sync example notebooks (.py example was outdated with respect to .ipynb)
  • Fix example on mathematical limits of model which was outdated

A few comments:

  • ConsPortfolioModel does not have enough tests for its different features.
    • Solver on discrete grid
    • Solver on Joint distribution

Please ensure your pull request adheres to the following guidelines:

  • 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.

reorganize solution method into object oriented solver
sync ipynb and py notebooks
fix section on mathematical limits of model
@project-bot project-bot bot added this to Needs Triage in Issues & PRs Jul 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #1047 (95c03d3) into master (d59269d) will increase coverage by 1.10%.
The diff coverage is 94.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1047      +/-   ##
==========================================
+ Coverage   72.53%   73.64%   +1.10%     
==========================================
  Files          68       68              
  Lines       10298    10411     +113     
==========================================
+ Hits         7470     7667     +197     
+ Misses       2828     2744      -84     
Impacted Files Coverage Δ
HARK/ConsumptionSaving/ConsPortfolioFrameModel.py 100.00% <ø> (ø)
HARK/ConsumptionSaving/ConsPortfolioModel.py 92.15% <93.58%> (+25.86%) ⬆️
...ConsumptionSaving/tests/test_ConsPortfolioModel.py 100.00% <100.00%> (ø)

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 d59269d...95c03d3. Read the comment docs.

@sbenthall
Copy link
Contributor

Just to make a note of my answer to a question you asked earlier ...

The best way to design 'unit tests' is to actually test the functionality of individual units of code, rather than test the output of many components put together (which is technically called an 'integration test').

So, if there are gaps in the test coverage, the best thing to do is drill down and write tests covering the various cases for those specific parts.

@sbenthall sbenthall changed the title ConsPortfolioModel Overhaul [WIP] ConsPortfolioModel Overhaul Aug 19, 2021
no need to set solve_one_period and update() as this is inherited from ConsPortfolioModel.py
add tests for new features: Discrete Share solution; Joint Distribution of Income and Risky Asset; Discrete Share and Joint Distribution
@alanlujan91 alanlujan91 changed the title [WIP] ConsPortfolioModel Overhaul ConsPortfolioModel Overhaul Oct 14, 2021
@alanlujan91
Copy link
Member Author

Updated and added basic tests for new features. I tried to make sure that it's compatible with Portfolio Frame functionality.

@sbenthall
Copy link
Contributor

Is this ready for review?

Is there a changelog entry for it in the PR?

I wonder if @Mv77 should review this, as he has done a lot of work on the ConsPortfolioModel.

@alanlujan91
Copy link
Member Author

Is this ready for review?

Is there a changelog entry for it in the PR?

I wonder if @Mv77 should review this, as he has done a lot of work on the ConsPortfolioModel.

Yes, this is ready for review.

I would appreciate a review from @Mv77, but also I want to make sure I don't mess up your Portfolio Frame functionality.

I will make some additions to changelog.

@sbenthall
Copy link
Contributor

Ok. I hope that both @Mv77 and I are able to review it.
I'll check for compatibility with my Frame work, hopefully by our regular Thursday meeting.

@sbenthall sbenthall self-assigned this Oct 19, 2021
@sbenthall
Copy link
Contributor

Ok, I have looked over this PR and see how it effects the work on FramedModels.

The changes to the underlying Portfolio model look good to me.

This PR has some merge conflicts with #1064 which seem unavoidable but minor.

@llorracc llorracc merged commit 4783977 into econ-ark:master Oct 21, 2021
Issues & PRs automation moved this from Needs Triage to Done Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Issues & PRs
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants