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 csv's to MANIFEST.in #957

Merged
merged 3 commits into from
Feb 21, 2021
Merged

Add csv's to MANIFEST.in #957

merged 3 commits into from
Feb 21, 2021

Conversation

Mv77
Copy link
Contributor

@Mv77 Mv77 commented Feb 20, 2021

My recent calibration tools added a couple datasets in csv format to the toolbox.

However, I had no idea these needed to be in the MANIFEST.in file to get collected when one installs HARK.

I noticed the issue while messing around with some conda environments for a cluster, and wondering why my tools were not working after installing HARK.

This tiny pr fixes the issue. It is my first time working with MANIFEST.in, could someone review?

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

@codecov-io
Copy link

Codecov Report

Merging #957 (59ba31c) into master (b85afd2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #957   +/-   ##
=======================================
  Coverage   71.43%   71.43%           
=======================================
  Files          62       62           
  Lines        9145     9145           
=======================================
  Hits         6533     6533           
  Misses       2612     2612           

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 b85afd2...59ba31c. Read the comment docs.

@llorracc llorracc requested a review from MridulS February 21, 2021 20:50
@llorracc
Copy link
Collaborator

@MridulS, this looks like one for you.

@MridulS
Copy link
Member

MridulS commented Feb 21, 2021

@Mv77 yup, need to put in these data files to make sure its packaged properly :)

@MridulS MridulS merged commit c6b3b22 into econ-ark:master Feb 21, 2021
@MridulS
Copy link
Member

MridulS commented Feb 21, 2021

@Mv77 Do you think it's possible to get a more flat structure in the datasets directory? Like moving all the csv files to the HARK/datasets/data folder and the data prep functions to HARK/datasets, next to the load_data.py file?

@MridulS
Copy link
Member

MridulS commented Feb 23, 2021

@Mv77 just wanted to bump this up.

sbenthall pushed a commit to sbenthall/HARK that referenced this pull request Feb 23, 2021
* Add csv's to manifest

* Update CHANGELOG.md

* Update MANIFEST.in
@Mv77
Copy link
Contributor Author

Mv77 commented Feb 23, 2021

I tried to get feedback on the location and format of the tools in my PR's (See #921, #922) and calls with @llorracc .

I proposed a format for datasets in which each had its folder with:

  • Data in as raw of a format as possible/sensible.
  • tools to parse the data into model params.
  • A readme explaining the format, variables and tools.

@llorracc and @sbenthall seemed to like it.

Flattening the dir structure is possible and would be advantageous in some dimensions, but it would go against concerns and principles that were brought up when I was creating the tools like:

  • Clutter. E.g., if we eventually add life tables for other countries in different formats, do we want them in the same folder?
  • The desire of data being as raw as possible.

I am not sure where the balance of those conveniences and concerns lands.

@MridulS
Copy link
Member

MridulS commented Feb 24, 2021

Thanks for the links, yeah I agree its a tricky balance between future proofing our code and conveniences.

Something like from HARK.datasets.SCF.WealthIncomeDist.SCFDistTools import income_wealth_dists_from_scf just seemed a bit too verbose on the first glance :)

@Mv77 Mv77 deleted the Calibration/Main branch April 29, 2021 15:32
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.

4 participants