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

Consumption Saving Refactor - move examples to notebooks - tests boilerplate #455

Merged
merged 27 commits into from
Feb 20, 2020

Conversation

MridulS
Copy link
Member

@MridulS MridulS commented Dec 12, 2019

I have removed the def main() code from ConsIndShockModel.py file and moved it to a example file.
This file will be present in the examples/ directory and parts of that file will be in the tests/ directory to create base level testing coverage of the module.

We also need to discuss what is exposed to the final user, for example ValueFunc, MargValueFunc and MargMargValueFunc are use by other classes to solve the model but I don't see users using this directly. Should this be exposed in the final documentation on readthedocs?

In this PR I'll repeat the same with all the other models and files in the ConsumptionSaving folder.

In this refactor I am also removing py2 specific dependencies in the ConsumptionSaving module as the py2 drop date is 15th Jan according to the roadmap.

@MridulS
Copy link
Member Author

MridulS commented Dec 12, 2019

related: #370
#440

@MridulS
Copy link
Member Author

MridulS commented Dec 12, 2019

[Build fails because it is running for py2.7 too]

@MridulS
Copy link
Member Author

MridulS commented Jan 16, 2020

Move def main to example_model.py

  • ConsAggShockModel.py
  • ConsPortfolioModel.py
  • ConsGenIncProcessModel.py
  • ConsPrefShockModel.py
  • ConsIndShockModel.py
  • ConsRepAgentModel.py
  • ConsMarkovModel.py
  • ConsMedModel.py
  • TractableBufferStockModel.py

@sbenthall
Copy link
Contributor

Would you like me to review this some time?

@MridulS
Copy link
Member Author

MridulS commented Jan 19, 2020

@sbenthall that would be great! Almost done with this one.

@sbenthall sbenthall mentioned this pull request Jan 20, 2020
@MridulS
Copy link
Member Author

MridulS commented Jan 23, 2020

@sbenthall Can you go through this once, I don't want to make more changes to this as it will create a big PR. I'll start a new PR with further changes.

@MridulS MridulS changed the title [WIP] Consumption Saving Refactor Consumption Saving Refactor - move examples to notebooks - tests boilerplate Jan 23, 2020
@sbenthall sbenthall self-assigned this Jan 23, 2020
@sbenthall
Copy link
Contributor

@MridulS Sorry for the slow action on this one.
I merged updates from master.
It looks like there are some tests failing.

@MridulS
Copy link
Member Author

MridulS commented Feb 5, 2020

Ah the consumption parameters change is failing the tests, I'll update this PR.

@MridulS
Copy link
Member Author

MridulS commented Feb 6, 2020

okay this is weirder than I thought, something is breaking between import structure of python3.6 and python3.7

This works for py3.7 and py3.8

@sbenthall sbenthall removed their assignment Feb 8, 2020
@sbenthall
Copy link
Contributor

Because the 3.6 build was broken on an issue involving import of the ConsumptionSaving files, and because there was a merge conflict with master over HARK/__init__.py, I took the liberty of trying to resolve the merge.

I removed this line:
from HARK.ConsumptionSaving import *

which seemed to be opening up a new way to import ConsumptionSaving files.
If I understand correctly, that line lets you do:
from HARK import MarkovConsumerType

instead of
from HARK.ConsumptionSaving import MarkovConsumerType

I see now that you used the former style (from HARK import MarkovConsumerType) elsewhere in the PR.
So I have to apologize for breaking something new.

A change I would test myself if I could use the different build environments in GitHub actions myself is whether if all these references are changed back to from HARK.ConsumptionSaving import XXXXXX, does that then pass the 3.6 build test.

@MridulS
Copy link
Member Author

MridulS commented Feb 20, 2020

reverted changes to make this py2.7 compatible and make it v0.10.4 friendly

@sbenthall sbenthall merged commit d1a8eb9 into econ-ark:master Feb 20, 2020
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