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

Move non-reusable model code to examples/ #442

Merged
merged 28 commits into from
Jan 5, 2020

Conversation

sbenthall
Copy link
Contributor

Addressing #440
Not ready for merge yet--not all the code functionality has been tested.

This is to show how it would work if the paper-specific research models were not HARK subpackages but rather were in an examples/ directory, an architectural pattern common to many scientific software libraries (matplotlib, networkx, Dolo).

I left HARK.ConsumptionSaving as is because that code is used by multiple downstream models.

There may be other reusable code from the other paper-specific models as well.
With this proposed architecture, reusable code would go into the HARK library as (sub)packages.
Code specific to the demonstration of a particular research result would go in examples/.

@llorracc
Copy link
Collaborator

llorracc commented Dec 5, 2019

There are now a few DemARKs that are essentially just documentation for HARK tools. Those should all be moved to a new "examples" directory that is part of the HARK structure. Some of them (but not all) have file names ending in "Doc."

@sbenthall sbenthall requested a review from MridulS December 9, 2019 21:03
@sbenthall
Copy link
Contributor Author

The current PR is aimed at cleaning up the HARK package by taking specific model/result code out of the HARK/ directory and into an adjacent examples/ directory that is intended for tutorial and documentation use.

I've reviewed each module for downstream dependencies. See this comment and following notes:
#440 (comment)

These three subdirectories of HARK/ need special mention:

  • ConsumptionSaving has significant generalizable code and is expected to stay as it is, though the main() method functionality will likely be moved to examples/ or elsewhere
  • cstwMPC remains as-is. There is an outstanding issue split cstwMPC into general tools and example code #449 to polish its generalizable parts as library features and put the paper-specific code in examples/ or elsewhere.
  • I removed most of SolvingMicroDSOPs but left HARK.SolvingMicroDSOPs/Calibration/EstimationParameters.py, which has downstream dependencies. The question of how to deal with parameters in the HARK library is a separate issue, tools for writing out parameters to a Dolang configuration file #446

While incomplete from the perspective of #440 (because there is still a lot of main() method code in the library), I believe this PR makes progress and is ready to be merged. Requesting review from @MridulS

@MridulS
Copy link
Member

MridulS commented Dec 9, 2019

The notebooks inside BayerLuetticke aren't updated with the new path.

running ipython OneAsset-HANK.py gives me

ModuleNotFoundError: No module named 'HARK.BayerLuetticke.Assets.One.FluctuationsOneAssetIOUsBond'

this should be changed to the correct path in examples.

running ipython BayerLuetticke_wrapper.py results in

Exception: Model instance does not have a solution stored. To simulate, it is necessary to run the `solve()` method of the class first.

and solve() isn't implemented for BayerLuettickeAgent, this error is present in the master branch too, https://github.com/econ-ark/HARK/issues/450

In LifecycleModelExample.py

 import HARK.SolvingMicroDSOPs.Calibration.EstimationParameters as Params    # Parameters for the consumer type and the estimation

should be

 import HARK.SolvingMicroDSOPs.EstimationParameters as Params    # Parameters for the consumer type and the estimation

@MridulS
Copy link
Member

MridulS commented Dec 10, 2019

This PR supersedes #335, removing StickyE from HARK.

@sbenthall
Copy link
Contributor Author

Thanks for the review @MridulS

The notebooks inside BayerLuetticke aren't updated with the new path.

I've now fixed these paths.

LifecycleModelExample.py

Instead of changing the path on the LifecycleModelExample, I changed the location of the EstimationParams.py file to be in a Calibration subdirectory. This is how it currently is in master.

While this is quite awkward, I wanted to leave it as unchanged as possible because it exposes how econ-ark has been using some conflicting standards/assumptions in its management of parameters.

A common pattern in REMARK-like uses of HARK is to put parameters in a Calibration directory as a Python file. This pattern was then copied into the HARK library, though it is not very Pythonic. Coming up with a cleaner way to do this is part of task #446

@sbenthall
Copy link
Contributor Author

Added fix for #451 here with seaborn as an extra dependency.
Rationale being that examples/ are ran in a development installation because they are part of documentation.
I could be persuaded otherwise.

@sbenthall
Copy link
Contributor Author

I've reverted my commit intended to address #451 and merged in the true fix from master

@sbenthall sbenthall mentioned this pull request Dec 12, 2019
@sbenthall
Copy link
Contributor Author

I've added ConsPortfolioModelDoc to examples/.

I fixed the formatting of the first markdown cell block.

I do get an error when running the flat .py file from ipython:

$ ipython ConsPortfolioModelDoc.py 
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
~/projects/econ-ark/DemARK/notebooks/ConsPortfolioModelDoc.py in <module>
    136 plt.axhline(lnpcct.MertSamCampVicShare, c='r')
    137 plt.ylim(0,1.05)
--> 138 plt.text((aMax-aMin)/4,lnpcct.MertSamCampVicShare-0.1,r'$\uparrow $ limit as  $m \uparrow \infty$',fontsize = 22,fontweight='bold')
    139 plt.show()
    140 

~/.virtualenvs/econ-ark/lib/python3.7/site-packages/matplotlib/pyplot.py in text(x, y, s, fontdict, withdash, **kwargs)
   2961         x, y, s, fontdict=None,
   2962         withdash=cbook.deprecation._deprecated_parameter, **kwargs):
-> 2963     return gca().text(x, y, s, fontdict=fontdict, withdash=withdash, **kwargs)
   2964 
   2965 

TypeError: text() missing 1 required positional argument: 's'

I get this error in the original DemARK as well, and I do not get it when running this in a notebook.
It appears to be due to the way the expression r'$\uparrow $ limit as $m \uparrow \infty$' is getting parsed. Maybe the notebook uses a LaTeX parser that command line ipython does not?

@llorracc
Copy link
Collaborator

Well, it's certainly something to do with LaTeX. I'm guessing that when running as a notebook you for some reason are getting different access to the command-line LaTeX than when running on the command line. Not sure why that would be, but if so it is also a potential danger in the AWS headless server environment.

@sbenthall
Copy link
Contributor Author

Requesting merge or review @MridulS

@llorracc
Copy link
Collaborator

I'm confused by the history above, which seems to have diverged from being about some issue about whether there are different LaTeX environments for notebooks vs ipython command-line execution, to being about moving this to examples. Did the LaTeX issue get resolved? If so, what was the resolution? In particular, do we need to avoid some particular LaTeX syntax to avoid similar future problems?

@sbenthall
Copy link
Contributor Author

@llorracc I think the confusion is due to how this PR suffered from scope creep.

  1. The original PR was to move model execution code out of the HARK package (what gets imported when you import HARK) and into an examples/ directory that is dedicated to tutorials and documentation.

  2. You then asked me to add some DemARKs to this directory as well. I made issue DemARK build recipes #459

  3. I tried (2), but discovered that one of the DemARKs you requested that I move has a LaTeX issue. I've just now made issue #463 to track this issue (Maybe I didn't before because until this PR is merged, it refers to code that isn't in this repository)

I think that work on #459 and #463 should follow the merge of this PR, so that the changes are as incremental as possible.

@sbenthall
Copy link
Contributor Author

This is ready to merge.

@llorracc
Copy link
Collaborator

llorracc commented Jan 5, 2020 via email

@sbenthall
Copy link
Contributor Author

This PR, #442, is ready to merge.

#442

@@ -16,7 +16,7 @@
getPercentiles, getLorenzShares, calcSubpopAvg, approxLognormal
from HARK.simulation import drawDiscrete
from HARK import Market
import HARK.cstwMPC.SetupParamsCSTW as Params
import SetupParamsCSTW as Params
Copy link
Member

Choose a reason for hiding this comment

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

@sbenthall this is the issue here for econ-ark/DemARK#83

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