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

Break scripts to code and tests #370

Closed
MridulS opened this issue Jul 23, 2019 · 6 comments
Closed

Break scripts to code and tests #370

MridulS opened this issue Jul 23, 2019 · 6 comments

Comments

@MridulS
Copy link
Member

MridulS commented Jul 23, 2019

A lot of files in HARK are currently python scripts (with a main() function to test the classes and functions in the file) rather than a "module".
For example if you look at the documentation of https://hark.readthedocs.io/en/latest/generated/HARK.interpolation.html#module-HARK.interpolation, https://hark.readthedocs.io/en/latest/generated/HARK.estimation.html, you would see that dangling main() function.

@llorracc
Copy link
Collaborator

So, I guess you think they should all be modules instead of scripts? Sounds right to me. I think their scriptyness dates from early days when we were trying to build in examples and testing in the files themselves rather than having standalone tests and examples.

In cases (if any) where the main() actually includes some content, we should make sure to port that content to an appropriate place (a DemARK or a test, probably) before deleting it.

@JackShiqiLi
Copy link
Contributor

I had a look at this, it seems that in almost all files in HARK, the main() only contains a warning like this,
Core
except for parallel.py that has a test in there and interpolation.py that I think is designed to have some examples for each type of interpolation, but has not finished yet.
Interpolation
If @MridulS is concerned about the main() under functions in the readthedocs webpage, I think all functions, class, etc are recognized by Sphnix and generated in the webpage automatically, to avoid that, I suggest that we can move the warnings directly into the if __name__ == "__main__": block (instead of having a function), and move the test into a new file if we still need it for further development of parallel.py. For the examples in interpolation.py, if they are there on purpose (as suggested by the warning message), I think we should also move it into the if __name__ == "__main__": block instead of a new file.

@MridulS
Copy link
Member Author

MridulS commented Sep 26, 2019

@JackShiqiLi my main intent is to remove all the main() as well if if __name__ == "__main__" block of code which should never be inside a library(individual files which implement models/helper code), otherwise it's a collection of scripts.
If you look at various models, majority of them has if __name__ == "__main__" block which tests the model by initiating it, playing with parameters and plotting. A grep gives me

./HARK/simulation.py:if __name__ == '__main__':
./HARK/interpolation.py:if __name__ == '__main__':
./HARK/FashionVictim/FashionVictimParams.py:if __name__ == '__main__':
./HARK/FashionVictim/FashionVictimModel.py:if __name__ == '__main__':
./HARK/tests/OpenCLtest.py:if __name__ == "__main__":
./HARK/tests/test_TractableBufferStockModel.py:if __name__ == '__main__':
./HARK/tests/test_modelcomparisons.py:if __name__ == '__main__':
./HARK/estimation.py:if __name__ == '__main__':
./HARK/core.py:if __name__ == '__main__':
./HARK/cstwMPC/SetupParamsCSTW.py:if __name__ == '__main__':
./HARK/cstwMPC/cstwMPC.py:if __name__ == '__main__':
./HARK/BayerLuetticke/ConsIndShockModel_extension.py:if __name__ == '__main__':
./HARK/BayerLuetticke/BayerLuetticke_wrapper.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/One/FluctuationsOneAssetIOUs.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/One/Luetticke_wrapper.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/One/FluctuationsOneAssetIOUsBond.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/One/SteadyStateOneAssetIOUs.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/One/SteadyStateOneAssetIOUsBond.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/Two/SteadyStateTwoAsset.py:if __name__ == '__main__':
./HARK/BayerLuetticke/Assets/Two/FluctuationsTwoAsset.py:if __name__ == '__main__':
./HARK/dcegm.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsMarkovModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsAggShockModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsPrefShockModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsMedModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsRepAgentModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsGenIncProcessModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/ConsIndShockModel.py:if __name__ == '__main__':
./HARK/ConsumptionSaving/TractableBufferStockModel.py:if __name__ == '__main__':
./HARK/utilities.py:if __name__ == '__main__':
./HARK/parallel.py:if __name__ == "__main__":
./HARK/cAndCwithStickyE/StickyE_NO_MARKOV.py:if __name__ == '__main__':
./HARK/cAndCwithStickyE/StickyE_MAIN.py:if __name__ == '__main__':
./HARK/SolvingMicroDSOPs/Calibration/EstimationParameters.py:if __name__ == '__main__':
./HARK/SolvingMicroDSOPs/Calibration/SetupSCFdata.py:if __name__ == '__main__':
./HARK/SolvingMicroDSOPs/do_all.py:if __name__ == '__main__':
./HARK/SolvingMicroDSOPs/Code/StructEstimation.py:if __name__ == '__main__':

All files should just have classes and functions, eg: https://github.com/econ-ark/HARK/blob/master/HARK/ConsumptionSaving/ConsPortfolioModel.py

Let me know if this makes sense.

@JackShiqiLi
Copy link
Contributor

@MridulS Yes it definitely makes sense. Previously I only looked in the HARK folder but not subfolders and thought the messages in main() were there on purpose such as telling economists who are new to Python the files do nothing themselves. Thank you for explaining.

@sbenthall
Copy link
Contributor

+1 to this issue. See #440 for related point

@sbenthall
Copy link
Contributor

This is fixed

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

No branches or pull requests

4 participants