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

Add solow model #74

Merged
merged 162 commits into from
Jan 7, 2015
Merged

Add solow model #74

merged 162 commits into from
Jan 7, 2015

Conversation

davidrpugh
Copy link
Contributor

Placeholder PR for merging a continuous-time version of he Solow growth model into quantecon.models. Basic primitives of the Solow model are an aggregate production function and dictionary of parameter values. I want the user to provide a sympy expression for aggregate production and some parameters, and then the class methods should set of the resulting differential equation compute the jacobian and pass these to the ivp.IVP class.

As always feedback is much appreciated!

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.07%) when pulling f1a81ba on davidrpugh:add-solow-model into a2c6bf4 on jstac:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.07%) when pulling f1a81ba on davidrpugh:add-solow-model into a2c6bf4 on jstac:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.87%) when pulling 5fdcc95 on davidrpugh:add-solow-model into a2c6bf4 on jstac:master.

@davidrpugh
Copy link
Contributor Author

@spencerlyon2

I am bamboozled by the following traceback that is generated when I run nosetests test_solow.py from the terminal...

======================================================================
ERROR: Testing validation of output attribute.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/drpugh/anaconda/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/drpugh/Research/python-dev/quant-econ/quantecon/tests/tests_models/test_solow.py", line 55, in test_validate_params
    mod = solow.model.Model(output=valid_output, params=valid_params)
AttributeError: 'module' object has no attribute 'model'

I have no trouble importing the quantecon.models.solow in the notebook and using creating instances using syntax like

mod = solow.model.Model(output=cobb_douglas_output, params=default_params)

Any ideas as to what is causing the issue? Perhaps the use of relative imports within the testing script?

@coveralls
Copy link

Coverage Status

Coverage increased (+1.41%) when pulling b5d6d4f on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.4%) when pulling 583b743 on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.41%) when pulling 15ba90f on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.41%) when pulling 15ba90f on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.36%) when pulling 4152045 on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.37%) when pulling 9d7a5a2 on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@davidrpugh
Copy link
Contributor Author

@jstac @spencerlyon2 @albop

After many trials and tribulations (including my family getting, temporarily, deported from the UK due to a visa snafu!) this PR is ready for a merge. Any feedback would be much appreciated.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.37%) when pulling f255f10 on davidrpugh:add-solow-model into f23a4b5 on QuantEcon:master.

@sglyon
Copy link
Member

sglyon commented Jan 6, 2015

@davidrpugh I have given this a decent look and everything seems to be in order. Great work. Good docstrings. Thorough tests. Well done.

I was wondering what people thought about having models/solow/other_files.py instead of just putting all the code related to this model in /models/solow.py. To me it doesn't make a huge difference. We have set a precedent with other models to be written in the same file, but that may be because the code for the other models is quite a bit shorter. Again, I don't feel it needs to be one way or the other, I just wanted to bring it up to start conversation in case someone else has a stronger opinion about this.

@davidrpugh
Copy link
Contributor Author

@spencerlyon2

Thanks for the feedback. It is my intention that much of the code for the Solow model will turn into base classes for general continuous time models. This is why 1) there is so much code; 2) I started dividing it into extra files.

@sglyon
Copy link
Member

sglyon commented Jan 6, 2015

@davidrpugh

Thanks for the quick response, that makes sense. I think for now (because we have no other continuous time models) this is probably good. If we need to refactor some things in the future when we have additional continuous time models and a better idea about optimal code location, we can revisit where the code should live.

@jstac
Copy link
Contributor

jstac commented Jan 7, 2015

@davidrpugh Man, that must have been some snafu. I hope it's all worked out OK.

Many thanks for finishing this contribution up. Everything seems well written and very nicely motivated. In my opinion this is ready to merge. Unless there's any comments in the next few hours I'll go ahead and do that.

@albop
Copy link
Contributor

albop commented Jan 7, 2015

@davidrpugh @spencerlyon2 and @jstac : It is a good pedagogical material with a lot of hard work inside. Given how long it has stayed in the pipeline my vote would go for merging the PR exactly as it is, with the several files. The next step is obviously to advertise the notebooks on quantecon.org .

As for turning it into a general purpose continuous time module, I am not 100% sure that it is the right base to build upon. After looking at the code for a while, a big part of it seems tailored to a special class of models, namely the Solow models and I don't understand very well how you could turn it into a generic tool without rewriting it significantly. In any case, it seems important to design a bit in advance what the goals of such a module would be and possibly to develop it outside of the main library for a while. Let's open an issue for that and discuss it there rather than in a PR.

@jstac
Copy link
Contributor

jstac commented Jan 7, 2015

@albop Thanks. I agree with those comments on possible development of a continuous time module.

Merging now.

jstac added a commit that referenced this pull request Jan 7, 2015
@jstac jstac merged commit d530db2 into QuantEcon:master Jan 7, 2015
@davidrpugh
Copy link
Contributor Author

@jstac

Excellent! Am glad to get this finally merged. @albop the impulse response class is fairly tailored to the solow models, but the underlying logic behind the _symbolic* and _numeric* methods can be refactored out to serve as a base for a generic continuous time model module. Most of the methods in the Model class are just syntactic sugar for pedagogic use.

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.

None yet

5 participants